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

Add support for LexicalHandler.startEntity and endEntity #210

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

philippn
Copy link
Contributor

This is the pull request for issue #209

It also adds a test case.

One thing that I find problematic at the moment is that it needs an extra configuration on the STAX factory. It would be better if this would either be the default behaviour -- which is a breaking change but might make sense since other SAX parsers behave that way too -- or at least could be controlled via a feature flag.

@cowtowncoder
Copy link
Member

Ok, so looks good (with brief look). But one thing I'll need before merging is CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(used for Woodstox too, not just Jackson)

and it's only needed before the first contribution. The usual way is to print it, fill & sign, scan/photo, email to cla at fasterxml dot com. Once I get that I can do final review and merge the fix.

@cowtowncoder cowtowncoder added the cla-needed PR ok (although may also require code review), but CLA needed from submitter label Aug 21, 2024
@philippn
Copy link
Contributor Author

Thanks for considering it. I have sent you the CLA.

@cowtowncoder cowtowncoder removed the cla-needed PR ok (although may also require code review), but CLA needed from submitter label Aug 22, 2024
@cowtowncoder
Copy link
Member

CLA received, can proceed later tonight.

@cowtowncoder
Copy link
Member

@philippn Ok, I will merge this now.

As to the part about awkward configurability: I am open to suggestions here.
Timing is bit unfortunate, as 7.0.0 was released quite recently -- change of defaults would have worked quite nicely if it was part of 7.x change. Less so for minor version (would need to go in 7.1.0). And of course default would just apply to SAX side.

Still: I am open to a follow-up PR for doing just that (or, if you want, feature flag to change it directly). I probably won't have time to look into that myself, but I'll always try to help with contributions so others can help.

@cowtowncoder cowtowncoder added this to the 7.1.0 milestone Aug 23, 2024
@cowtowncoder cowtowncoder merged commit a2ac3ef into FasterXML:master Aug 23, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants