DO NOT REPLY [Bug 34828] - [PATCH] FormulaEvaluator Partial Implementation

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG?
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=34828>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND?
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=34828





------- Additional Comments From [hidden email]  2005-05-10 16:43 -------
**** **** **** **** **** **** **** **** ****  ****
**** Note: I goofed up a bit on the second    ****
**** version of the patch #14979              ****
**** As a result, you may have to apply the   ****
**** patch to /src/scratchpad/src instead of  ****
**** the root folder                          ****
**** **** **** **** **** **** **** **** ****  ****

Avik, Thanks for the encouraging response

1. Ptg and Eval: Delegating or extending?
Initially I had an impl that added hooks into *Ptgs to get/store values.
But with input from you and Andy I realised that integrating tightly with Ptgs
would mean that the code could not go in until it was completely tested (read a
long long time :)

IMHO, having Evals separate from Ptg is not too bad since there are only so many
*Evals and  all the important Evals have been covered. Delegation has the
advantage (in this case atleast) of decoupling FormulaParser from
FormulaEvaluator. But I guess extending from Ptg could also work...

2. Functions in one class or one class per function?
Honestly, I did not give this much thought when I designed - I just assumed one
class per function was superior. Thinking about it now, it seems like multiple
functions in one class is an alternative - though wrt maintainability i think it
would be easier to test-develop-maintain individual classes than one big class.
Some of the function implementations themselves can be quite big themselves, so
one huge class would be /really/ huge. Also one function per class should make
it easier to organize unit tests.
Unless one class per function causes problems, I'd vote for one class per function.

3. Testing...
Unit testing would be a big effort. Almost as big as writing individual
functions, a distributed effort would be great!

4. My eclipse...
Yeah, sorry about that. I have two different versions of eclipse, I still havent
taught one of them not to do the default comments :)

5. Under contruction:
The work on core eval classes is not yet "complete". I think there is need for a
BlankEval class to handle empty cells - which are currently being handled as
StringEval("").

--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

[Moving to the list, since bugzilla is bad for conversation]

Comments inline.

> **** As a result, you may have to apply the   ****
> **** patch to /src/scratchpad/src instead of  ****
> **** the root folder                          ****

That's ok. For the initial bit, can you just zip/tgz the directory. Provide
patches after the initial additions have been comitted. I am waiting to commit
till the dummy method javadocs are removed (maybe an awk script might be
quick?)

>
> 1. Ptg and Eval: Delegating or extending?
> <snip> But I guess extending from Ptg could also work...

Just to clarify, by extend, i meant have a separate class that interits
from the
Ptgs.. ie, `class AddEval extends AddPtg` .. but as I said, just a suggestion,
you'll know best.

> 2. Functions in one class or one class per function?
<snip>

Yeah, sure.

> 3. Testing...
> Unit testing would be a big effort. Almost as big as writing individual
> functions, a distributed effort would be great!

Certainly, probably bigger. So once again, this is a call for everybody
interested in this functionality to write unit tests for it. Its easy!

>
> 4. My eclipse...
<snip>

Please try and remove the extraneous comments, i dont want to put that in CVS.
I'll commit it as soon as that's done, I'm happy with everything else.

> 5. Under contruction:
> The work on core eval classes is not yet "complete". I think there is
> need for a
> BlankEval class to handle empty cells - which are currently being handled as
> StringEval("").

Yeah, sure, thats understood, and no issues. But your 'empty cell'  
comment made
me think, the function implementations should probably follow Excel
semantics..
for eg, in the average function, empty cells do not add to the number of
elements (ie, the denominator). Also, these semantics might be different for
different versions of excel. How do we handle that.

A final thoguht on error handling. Your error handling is modelled after OO.o.
In excel however, errors are a more limited set.. #REF, #VALUE, etc. Do
we need
to think about how to handle/map those. Maybe it doesnt matter?

Once again, to everybody out there, this is a big and important piece of work
that can certainly do with more evaluation/comments/unit tests. Thanks!

Regards
-
Avik



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[andy-2]

-1 on committing any patches without unit tests anymore.  It
destabilizes code too much.  Even to scratchpad because then the code
never moves out of scratchpad.  We learned this lesson already.  It
doesn't matter how good or bad or how much we want it...no unit tests +
doco...it stays as patches in bugzilla.

[hidden email] wrote:

> [Moving to the list, since bugzilla is bad for conversation]
>
> Comments inline.
>
>> **** As a result, you may have to apply the   ****
>> **** patch to /src/scratchpad/src instead of  ****
>> **** the root folder                          ****
>
>
> That's ok. For the initial bit, can you just zip/tgz the directory. Provide
> patches after the initial additions have been comitted. I am waiting to
> commit
> till the dummy method javadocs are removed (maybe an awk script might be
> quick?)
>
>>
>> 1. Ptg and Eval: Delegating or extending?
>> <snip> But I guess extending from Ptg could also work...
>
>
> Just to clarify, by extend, i meant have a separate class that interits
> from the
> Ptgs.. ie, `class AddEval extends AddPtg` .. but as I said, just a
> suggestion,
> you'll know best.
>
>> 2. Functions in one class or one class per function?
>
> <snip>
>
> Yeah, sure.
>
>> 3. Testing...
>> Unit testing would be a big effort. Almost as big as writing individual
>> functions, a distributed effort would be great!
>
>
> Certainly, probably bigger. So once again, this is a call for everybody
> interested in this functionality to write unit tests for it. Its easy!
>
>>
>> 4. My eclipse...
>
> <snip>
>
> Please try and remove the extraneous comments, i dont want to put that
> in CVS.
> I'll commit it as soon as that's done, I'm happy with everything else.
>
>> 5. Under contruction:
>> The work on core eval classes is not yet "complete". I think there is
>> need for a
>> BlankEval class to handle empty cells - which are currently being
>> handled as
>> StringEval("").
>
>
> Yeah, sure, thats understood, and no issues. But your 'empty cell'  
> comment made
> me think, the function implementations should probably follow Excel
> semantics..
> for eg, in the average function, empty cells do not add to the number of
> elements (ie, the denominator). Also, these semantics might be different
> for
> different versions of excel. How do we handle that.
>
> A final thoguht on error handling. Your error handling is modelled after
> OO.o.
> In excel however, errors are a more limited set.. #REF, #VALUE, etc. Do
> we need
> to think about how to handle/map those. Maybe it doesnt matter?
>
> Once again, to everybody out there, this is a big and important piece of
> work
> that can certainly do with more evaluation/comments/unit tests. Thanks!
>
> Regards
> -
> Avik
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
> .
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Rainer Klute (Rainer Klute IT-Consulting)]

Am Dienstag, den 10.05.2005, 20:38 -0700 schrieb
[hidden email]:
> -1 on committing any patches without unit tests anymore.  It
> destabilizes code too much.  Even to scratchpad because then the code
> never moves out of scratchpad.  We learned this lesson already.  It
> doesn't matter how good or bad or how much we want it...no unit tests +
> doco...it stays as patches in bugzilla.

I second that. Better fewer functionality but tested and documented than
vice versa.

Best regards
Rainer Klute

                           Rainer Klute IT-Consulting GmbH
  Dipl.-Inform.
  Rainer Klute             E-Mail:  [hidden email]
  K?rner Grund 24          Telefon: [phone]
D-44143 Dortmund           Telefax: [phone]

Inhibit software patents: http://www.nosoftwarepatents.com/


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Amol Deshmukh-2]

--- [hidden email] wrote:

> -1 on committing any patches without unit tests
> anymore.  It
> destabilizes code too much.  Even to scratchpad
> because then the code
> never moves out of scratchpad.  We learned this
> lesson already.  It
> doesn't matter how good or bad or how much we want
> it...no unit tests +
> doco...it stays as patches in bugzilla.
>

Wow... Ok. Fine by me.

But...

* Unit tests:
do you mean *all* unit tests, some unit tests that
test
basic flows or unit tests with complete code coverage?

(I'm asking for the min criteria to get this into
scratchpad)
I hope you mean its ok to have failing unit tests
for functionality that is not yet implemented?

Also I guess that means that I'm on my own till the
code moves into CVS? Again, fine by me, but if there
are any ideas to enable distributed development w/o
putting it into CVS, they are welcome.

* Docos...
I assume javadocs. Correct me if you meant
otherwise...


~ amol





---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

On Tue, 2005-05-10 at 20:38 -0700, [hidden email] wrote:
> -1 on committing any patches without unit tests anymore.  It
> destabilizes code too much.  Even to scratchpad because then the code
> never moves out of scratchpad.  We learned this lesson already.  It
> doesn't matter how good or bad or how much we want it...no unit tests +
> doco...it stays as patches in bugzilla.
>

While I agree with your general sentiments, I think that not getting
things into CVS detracts from getting more contributors, particularly
for this formula stuff, where unit tests are going to be easy to write,
but we need lots and lots of it, and one person is certainly not going
to finish all cases. That's why I think we have scratchpad, so that
people can start looking at incomplete stuff.

If required, we should have criteria for removing stuff from scratchpad,
if they remain unmaintained for too long. Also we should NEVER release
jars of stuff from scratchpad, only CVS. If scratchpad and main have the
same criteria, then why have scp? would you rather that HWPF not have
been added to CVS, as opposed to being where it is now? (sorry, too many
rhetorical q's :)

Amol, irrespective of this disucussion, you should start writing some
tests for your code :)

Regards
-
Avik

> [hidden email] wrote:
> > [Moving to the list, since bugzilla is bad for conversation]
> >
> > Comments inline.
> >
> >> **** As a result, you may have to apply the   ****
> >> **** patch to /src/scratchpad/src instead of  ****
> >> **** the root folder                          ****
> >
> >
> > That's ok. For the initial bit, can you just zip/tgz the directory. Provide
> > patches after the initial additions have been comitted. I am waiting to
> > commit
> > till the dummy method javadocs are removed (maybe an awk script might be
> > quick?)
> >
> >>
> >> 1. Ptg and Eval: Delegating or extending?
> >> <snip> But I guess extending from Ptg could also work...
> >
> >
> > Just to clarify, by extend, i meant have a separate class that interits
> > from the
> > Ptgs.. ie, `class AddEval extends AddPtg` .. but as I said, just a
> > suggestion,
> > you'll know best.
> >
> >> 2. Functions in one class or one class per function?
> >
> > <snip>
> >
> > Yeah, sure.
> >
> >> 3. Testing...
> >> Unit testing would be a big effort. Almost as big as writing individual
> >> functions, a distributed effort would be great!
> >
> >
> > Certainly, probably bigger. So once again, this is a call for everybody
> > interested in this functionality to write unit tests for it. Its easy!
> >
> >>
> >> 4. My eclipse...
> >
> > <snip>
> >
> > Please try and remove the extraneous comments, i dont want to put that
> > in CVS.
> > I'll commit it as soon as that's done, I'm happy with everything else.
> >
> >> 5. Under contruction:
> >> The work on core eval classes is not yet "complete". I think there is
> >> need for a
> >> BlankEval class to handle empty cells - which are currently being
> >> handled as
> >> StringEval("").
> >
> >
> > Yeah, sure, thats understood, and no issues. But your 'empty cell'  
> > comment made
> > me think, the function implementations should probably follow Excel
> > semantics..
> > for eg, in the average function, empty cells do not add to the number of
> > elements (ie, the denominator). Also, these semantics might be different
> > for
> > different versions of excel. How do we handle that.
> >
> > A final thoguht on error handling. Your error handling is modelled after
> > OO.o.
> > In excel however, errors are a more limited set.. #REF, #VALUE, etc. Do
> > we need
> > to think about how to handle/map those. Maybe it doesnt matter?
> >
> > Once again, to everybody out there, this is a big and important piece of
> > work
> > that can certainly do with more evaluation/comments/unit tests. Thanks!
> >
> > Regards
> > -
> > Avik
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> > The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
> > .
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
>

--


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

Quoting Amol Deshmukh <[hidden email]>:

>
> But...
>
> * Unit tests:
> do you mean *all* unit tests, some unit tests that
> test
> basic flows or unit tests with complete code coverage?

not all, some tests, testing the basic functionality.

> Also I guess that means that I'm on my own till the
> code moves into CVS? Again, fine by me, but if there
> are any ideas to enable distributed development w/o
> putting it into CVS, they are welcome.
>
> * Docos...
> I assume javadocs. Correct me if you meant
> otherwise...

javadocs for the primary methods.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

Quoting [hidden email]:

> Quoting Amol Deshmukh <[hidden email]>:
>
>>

>> * Docos...
>> I assume javadocs. Correct me if you meant
>> otherwise...
>
> javadocs for the primary methods.
>

And a basic how-to .. how to use the api. Even if only a few lines.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Amol Deshmukh]

TWIC:

I've started writing automated tests and
I can see how they are immensely helpful :)

So this is my TODO list for now:
1. Write automated tests for the core classes
  (all OperatorEval impls)
2. Do a few basic functions each with its own set of
  automated tests as a proof-of concept that functions
  will work.
3. Get all classes in pkg xxxxx.formula.eval to
  pass all unit tests.


I have written the user api docs and contributors
guide which in itself was a GoodThing, since that
made me revisit the user api and change it to make
it (slightly) more usable.

So once I'm done with my TODO list I will submit
a patch along with the user and dev docs. Will
probably take me another weekend though, so expect
(if u care that is :) the next patch around Monday.

Hopefully with a couple of more rounds of testing,
IMHO, it should be in good shape to be worthy of
getting into scratchpad (?)

Thanks and Regards,
~ amol

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: Wednesday, May 11, 2005 2:11 PM
> To: [hidden email]
> Subject: Re: FormulaEvaluator Partial Implementation
>
>
>
> Quoting [hidden email]:
>
> > Quoting Amol Deshmukh <[hidden email]>:
> >
> >>
>
> >> * Docos...
> >> I assume javadocs. Correct me if you meant
> >> otherwise...
> >
> > javadocs for the primary methods.
> >
>
> And a basic how-to .. how to use the api. Even if only a few lines.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

On Thu, 2005-05-12 at 08:54 -0400, Amol Deshmukh wrote:
> TWIC:
>
> I've started writing automated tests and
> I can see how they are immensely helpful :)
>
See, told you so! ha ha..! :)

<snip>

> I have written the user api docs and contributors
> guide which in itself was a GoodThing, since that
> made me revisit the user api and change it to make
> it (slightly) more usable.
>
Great! have you written them as xdocs? If you want, post them to
bugzilla, and I'm happy to convert them to xdocs. You can then maintain
them in that format later.

> So once I'm done with my TODO list I will submit
> a patch along with the user and dev docs. Will
> probably take me another weekend though, so expect
> (if u care that is :) the next patch around Monday.
>
I'm sure many people do.

> Hopefully with a couple of more rounds of testing,
> IMHO, it should be in good shape to be worthy of
> getting into scratchpad (?)
>
I certainly think so. Might be useful to put your wip in bugzilla, if it
isnt too much of a pain, I can pitch in if I have some time.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Amol Deshmukh-2]

>
> ------- Additional Comments From [hidden email]
2005-05-12
> 20:38 -------
> Another thought, on the tests... Why do you need
separate
> test methods/classes
> for each eval if you are directly reading it from
the excel

<snip/>

> That way,
> one method (with asserts in a loop) will test all
> functions/operators.


...because otherwise I wouldnt know which test failed
if even one test failed :)

Also this way we get more control over which tests we
want to carry out simply by specifying the required
classes in a testsuite like AllTests.java.

Effortwise it is not a big deal since I generated all
the classes using code generation (you didnt expect me

to /write/ all the test classes did you? :)

If there are suggestions to improve after you see the
test framework and the test cases, I'll be glad to
make
necessary changes...

Thanks and Regards,
~ amol


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

Some comments inline.

On Thu, 2005-05-12 at 12:30 -0700, Amol Deshmukh wrote:

> >
> > ------- Additional Comments From [hidden email]
> 2005-05-12
> > 20:38 -------
> > Another thought, on the tests... Why do you need
> separate
> > test methods/classes
> > for each eval if you are directly reading it from
> the excel
>
> <snip/>
>
> > That way,
> > one method (with asserts in a loop) will test all
> > functions/operators.
>
>
> ...because otherwise I wouldnt know which test failed
> if even one test failed :)
>

No, you would... you'll get the formula from excel, and each formula
would be a separate assert, and in the assert comment, you put the
formula. so:

while (get all cells) {
        ...
        assertEquals(c.getCellFormula(), expectedValueCell, actualValue)

}

> Also this way we get more control over which tests we
> want to carry out simply by specifying the required
> classes in a testsuite like AllTests.java.
>
Well, if you are doing test first development that helps, but really,
for regression testing, doesnt matter much, so long as you can see what
is failing.

And while implementing other functions, if someone wants to do
test-first, its better to write a lower level test without reading from
an excel file, but doing everything in POI.

> Effortwise it is not a big deal since I generated all
> the classes using code generation (you didnt expect me
>
> to /write/ all the test classes did you? :)
>
No i didnt.... :) but I am thinking less code to maintain! A year later,
a new contributor will forget about generation, and copy paste...

> If there are suggestions to improve after you see the
> test framework and the test cases, I'll be glad to
> make
> necessary changes...
>
> Thanks and Regards,
> ~ amol
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
>

--


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Amol Deshmukh]

> > > ------- Additional Comments From [hidden email]
> > 2005-05-12
> > > 20:38 -------
> No, you would... you'll get the formula from excel, and each formula
> would be a separate assert, and in the assert comment, you put the
> formula. so:
>
> while (get all cells) {
> ...
> assertEquals(c.getCellFormula(), expectedValueCell, actualValue)
>
> }

But what happens when one assert fails - wouldnt the
AssertionError cause the cells that follow to never be
tested. (BTW, I'm a JUnit novice, so I may be wrong
- in which case we can even continue this discussion
off the list to avoid some embarassment to me ;)

But I see your point here - I could have a single class
& init it for every test case instead of creating new
class per function - the issue is maintainability and
size of code base.

Definitely, like you say my current approach seems
to be overkill & not pleasant to maintain...
...I will make changes as per your suggestion in the
automated tests.

Thanks for your input!
~ amol

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/

[Avik Sengupta]

> But what happens when one assert fails - wouldnt the
> AssertionError cause the cells that follow to never be
> tested. (BTW, I'm a JUnit novice, so I may be wrong
> - in which case we can even continue this discussion
> off the list to avoid some embarassment to me ;)
>
No, you are correct, but my point is when you are doing regression
tests, it doesn't matter. The idea is that our regression tests pass
100% for all code in CVS, so if one test fails, you fix that, and then
if another fails, you fix that...

Obviously, if you are doing test-first, it doesnt work very well, but in
that case, you'll be writing different tests in any case.

> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
> The Apache Jakarta POI Project: http://jakarta.apache.org/poi/
>
--


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
Mailing List:    http://jakarta.apache.org/site/mail2.html#poi
The Apache Jakarta POI Project: http://jakarta.apache.org/poi/