Fix, document, and test ColumnType.compare and Table.compareRows #1266
Conversation
ccleva
force-pushed
the
fix_table_comparerows
branch
from
73c9c25 to
46445a3
Compare
|
All conversation should be fixed. I rebased the branch also in case |
|
Can you rebase this PR again to make sure the formatter runs against it now that the formatter's been fixed? |
|
I'm unable to make the formatter working for the moment |
|
@ccleva could you try rebasing and formatting now that you've gotten the formatter working? |
ccleva
force-pushed
the
fix_table_comparerows
branch
from
46445a3 to
92fdb67
Compare
Yes, I rebased then ran the hook manually |
| * Compares the row at {@code rowNumber} in {@code column1} and {@code column2} and returns whether they are equal. | ||
| * @throws {@code IndexOutOfBoundsException} if {@code rowNumber} exceeds either column size | ||
| */ | ||
| default boolean compare(int rowNumber, Column<?> column1, Column<?> column2) { |
There was a problem hiding this comment.
this feels like kind of a weird method honestly. I wonder if we even need it to be a standalone method or if we can just inline it into compareRows
There was a problem hiding this comment.
It might be used by people for other purposes. No harm in keeping it I think.
There was a problem hiding this comment.
We're changing the functioning of it pretty drastically. Anyone who is relying on it is going to be broken. Better for them to find that out at compile-time because we removed it than at runtime
There was a problem hiding this comment.
Ah right, this one was broken in the previous version anyway, good point. Will change as you suggest
* Fix document and test ColumnType.compare and Table.compareRows * Fix sonar comment * More explicit test name
ccleva
force-pushed
the
fix_table_comparerows
branch
from
92fdb67 to
90c3083
Compare
Thanks for contributing.
Description
Fixes #1229
Testing
Yes, let me know if you want them moved to an existing test class