tabula-java allow specifying multiple areas#213
Conversation
…#1942) - Allow multiple occurrences of -a parameter - Allow -a parameter to accept % values as well as absolute values - Add test cases - Add test files
| return getArea(area, mode); | ||
| } | ||
|
|
||
| public Page getArea(Rectangle area, int mode) { |
There was a problem hiding this comment.
I'd rather leave this method as it was, and make the relative→absolute conversion in CommandLineApp.
There was a problem hiding this comment.
The orginal method is there as is... I didn't modify it. I simply added another method.
Additionally based on the comment at the top of the file...
// TODO: this class should probably be called "PageArea" or something like that
it seemed to me that Page.java is the place where all area calculations should be housed and hence I decided the add that method.
I can anyways move the calculations out of Page.java... but just want a quick confirmation from you again if possible please.
There was a problem hiding this comment.
Sorry about that. I misread and thought you modified the original method. My bad.
In any case, I still feel that the conversion belongs in CommandLineApp.
| if (pageArea != null) { | ||
| page = page.getArea(pageArea); | ||
| if (pageAreas != null) { | ||
| for (Pair<Integer, Rectangle> areaPair : pageAreas) { |
There was a problem hiding this comment.
I mean here. You have access to the Page objects, so you can do the conversion at this point.
There was a problem hiding this comment.
Agree... tried to explain the thinking behind why I put the calculations the Page.java as opposed to CommandLineApp.java file... Please do confirm once more before I send the updated code.
PS: I apologize for asking to reconfirm
There was a problem hiding this comment.
Explanation is under the other review comment.
There was a problem hiding this comment.
Changes done and committed.
|
This is awesome, @asheeshrana — Thanks a lot! The tests are very much appreciated :) I've requested a couple minor changes, let me know what you think. |
- Moved the constants out of page class
|
Merged! Thanks a lot, @asheeshrana ! |
2 participants
1942-update-tabula-java-for-muti-column-pdf (PolicyReporter/requests/#1942)
Misc Notes for reviewer:
This change is