Patch averageif #330
Patch averageif #330
Conversation
| */ | ||
| public class AverageIf | ||
| extends Baseifs | ||
| { |
There was a problem hiding this comment.
We try to follow a code-style where brackets are on the previous line, can you re-format this class to follow that, also "extends" is usually on the same line as the class, i.e.
public class AverageIf extends Baseifs {
There was a problem hiding this comment.
ok. i've never been a fan of K&R formatting, but i can make that change.
|
|
||
| /** | ||
| * Example 1 from | ||
| * https://support.microsoft.com/en-us/office/maxifs-function-dfd611e6-da2c-488a-919b-9b6376b28883 |
There was a problem hiding this comment.
should this link be https://support.microsoft.com/en-us/office/averageif-function-faec8e2e-0dec-4308-af69-f5576d8ac642 ? the test data does not match either of the examples on that doc page though
There was a problem hiding this comment.
yes. the danger of cut/paste coding (i stole the UT from averageifs) for expedience. i can make the change.
There was a problem hiding this comment.
can you use the examples from https://support.microsoft.com/en-us/office/averageif-function-faec8e2e-0dec-4308-af69-f5576d8ac642 though - ie the data and test scenarios?
There was a problem hiding this comment.
the M$ examples are rubbish - very focused on a small number of happy path cases and occasionally an exception case but at least using them as a guide is better than ignoring them and focusing a use case that suits you and maybe noone else
|
could you cover example2 from M$ too? |
|
and you ignored the review comment asking you to respect the code format |
|
ok. but i'm not aware of "ignoring" the code format request. i moved all the braces and other bits. |
|
I've merged what you've done so far after reformatting the code but with example2 being tested, the new code is suspect and liable for removal - 8a0d0d7 |
|
i was in the process of adding ex.2 to the test. should i continue? |
|
new code is incorrect - I have provided a pull request to your repo - if the example2 is left broken, i will remove all the new code |
|
Can you close this PR? - all this code is merged to Apache trunk and I have been fixing your bugs and code formatting issues. |
|
i do not appear to have that privilege. i cannot find the close button. you have my permission to close it if you have that privilege. |
|
I don't have a direct way to close this. I have un ugly way to close it but may avoid it. It's more important that you verify the apache poi trunk code to verify it works for you. |
|
no worries, i forgot that i forked it under a different user-id. closed |
adds missing support for singular AverageIf function