Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend scope of @ExcelCellName and @ExcelCell 'mandatory' flag to constrain cell values. #233

Closed
rob-heaney-itg opened this issue Mar 22, 2022 · 32 comments

Comments

@rob-heaney-itg
Copy link

Hi,

It would be great if the scope of the "mandatory" constraint (on the @ExcelCellName and @ExcelCell annotations) could be extended to affect the actual cell values, not just column headers. Is this possible?

Either that, or perhaps add support to PoijiOptions to allow for instantiating POJOs through our own all-arg-constructors, so that we can effectively add our own mandatory/optional validation during each row/cell creation, perhaps?

Not sure which is easier, but open to ideas if there are alternatives.
Thanks in advance.

@github-actions
Copy link

Thank you for contributing to Poiji! Feel free to create a PR If you want to contribute directly :)

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Mar 24, 2022

Hi @ozlerhakan,

If it helps, I have created a "draft" working example of how this could be approached. However, it's by no means production code, nor with the appropriate unit test coverage, just purely a concept demonstration. So rather than muddy your project repo with branches & pull-requests, is there somewhere I can provide you with a "work-in-progress" patch file for you to review?

Thanks

@ozlerhakan
Copy link
Owner

ozlerhakan commented Mar 24, 2022

Hi @rob-heaney-itg ,

Thank you for contributing to Poiji!

I come up with two options for this case:

  1. just fork Poiji do your changes on top of the master in a new branch and we can see how it goes on your forked repo.
  2. just fork Poiji do your changes on top of the master in a new branch and create a patch, and send it to me. I can review and send you a reviewed patch.

What do you think?

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Mar 24, 2022

Hi @ozlerhakan,

That sounds great. Please see attached below.

ISSUE-233___WORKING_EXAMPLE.patch.txt

Thank you for your consideration.

@ozlerhakan
Copy link
Owner

Thank you @rob-heaney-itg , I will look at it in 3 days.

@ozlerhakan
Copy link
Owner

Hi @rob-heaney-itg , It looks promising your changes, these changes are for xls files only, do you want to add the same ability for xlsx files? BTW I opened up a new branch called 3.1.8. You can make your PR on this branch. Thank you again! :) we can tune a bit more once you make your changes on code.

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Mar 28, 2022

Hi @ozlerhakan,

I did have a look into how to do the equivalent for XLSX files with the XSSFUnmarshaller, but in all honesty, I couldn't quite see where it could be done. Correct me if I'm wrong, but all the parsing in the XSSFUnmarshaller seems to be completely delegated to whichever SAXParser implementation you have on your classpath...? So it seems difficult to configure in the same way.

Besides, I am personally parsing my files separately and extracting each Sheet first, before passing to Poiji, so I'm only ever utilising the SheetUnmarshaller, which looks to be based solely on the HSSFUnmarshaller.

Again though, I'm open to new ideas, if you can see an approach that I've missed...?

@ozlerhakan
Copy link
Owner

ah I see @rob-heaney-itg , we can first go for XLS files then we can take another look at how we can do for XLSX files however, It might not be cover the same approach as XLSs.

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan , I don't appear to have permission to push commits to your 3.1.8 branch.

Is it best for you to commit my patch for me please?

Thanks

@ozlerhakan
Copy link
Owner

you can create a PR, the base branch will be 3.1.8. so the changes will be merged to 3.1.8 not master, or I can commit your patch no problem.

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Apr 10, 2022

Yes please @ozlerhakan, I would appreciate if you can commit, thank you.

@hoemich
Copy link

hoemich commented Apr 11, 2022

Would it be possible to differentiate between a mandatory header and a mandatory value?
We use this useful library to parse XLSX-files which are generated by some legacy systems.
We have columns with mandatory values but no header.
And @ozlerhakan could you consider - for such backward incompatible changes - to increase more than just the patch version? We were quite surprised how much code change has been necessary when increasing version from 3.1.1 to 3.1.5, when the default behavior for column headers suddenly changed.

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Apr 11, 2022

Would it be possible to differentiate between a mandatory header and a mandatory value?

Yes, I do agree actually, it would be much better to support separate flags in the annotation, rather than a global flag inside the PoijiOptions, as originally suggested.

Something like this perhaps:
@ExcelCellName(value = "myColumnName", mandatoryHeader = false, mandatoryCell = false)
or
@ExcelCellName(value = "myColumnName", mandatory = [HEADER|CELL|HEADER_AND_CELL|NONE])

@ozlerhakan
Copy link
Owner

I see @hoemich , we might go for what @rob-heaney-itg suggests.

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan , do you have a target time-frame as to when you could include this in your next release?

@ozlerhakan
Copy link
Owner

probably next week @rob-heaney-itg

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan , any update at all? Thanks

@ozlerhakan
Copy link
Owner

Hi @rob-heaney-itg ,

I'm on holiday, I need to schedule for the changes

@rob-heaney-itg
Copy link
Author

Not a problem @ozlerhakan , hope you enjoyed your holiday.

Would be good to know when this gets scheduled in, if you don't mind keeping us in the loop?

Many thanks

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan , any update on this?

Thanks

@ozlerhakan
Copy link
Owner

I want to close this issue ASAP @rob-heaney-itg , sorry

@rob-heaney-itg
Copy link
Author

rob-heaney-itg commented Aug 17, 2022

Thanks for the update @ozlerhakan 👍 Appreciate the commitment 😃

However, I've just noticed you've updated to Java 17 now ..... I really wish this issue had been released before the Java upgrade was, because the project we're using this on is stuck on Java 11 (with no possibility to upgrade to 17 any time soon!) .... Any chance of reverting the Java17-upgrade and only merging after this issue is released? Is that possible?

@ozlerhakan
Copy link
Owner

Hi @rob-heaney-itg ,

We can provide it for both v3.1.7 and the latest version.

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan, ok thank you, that'd be great. If that's the case, then yes I'd appreciate a Java11 version once this issue is merged/released. Many thanks again.

@rob-heaney-itg
Copy link
Author

Hi @ozlerhakan , is there any update on this, by any chance?

@ozlerhakan
Copy link
Owner

Hi @rob-heaney-itg , I extremely get around to it at the xmas week :)

@ozlerhakan
Copy link
Owner

We'll leverage Java 11 features again rather than Java 17.

@ozlerhakan
Copy link
Owner

Hi @rob-heaney-itg , do you have any null cell example?

@ozlerhakan
Copy link
Owner

First off, I'm sorry for being so late @rob-heaney-itg thank you for your support and priceless patience @rob-heaney-itg @hoemich 🙏 you can use 3.2.0 at the moment.

@valtsmazurs
Copy link

As this was backwards-incompatible change, perhaps it would bring additional clarity, if the project followed SemVer and bumped the major version number to 4 instead of the minor version to 2.

@ozlerhakan
Copy link
Owner

Hi @valtsmazurs , I see the point, let me upgrade the version to 4.0.0 that would be more practical based on your advise. Thank you.

@rob-heaney-itg
Copy link
Author

Thanks for getting this through @ozlerhakan, especially over the holiday period.
Apologies, I hadn't seen your other messages, but thanks for supporting this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants