-
-
Notifications
You must be signed in to change notification settings - Fork 919
Fix MRI Ruby vs. JRuby XML child namespace output differences #3456
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
Conversation
@flavorjones please approve CI, thanks! |
I'm not opposed to this approach. I'm wondering if the namespace stack should be managed in the |
@flavorjones I think you're right, I'll look more into getAttrsAndNamespaces tomorrow |
Thank you so much for diving in and drafting this pull request! I sincerely wish there were more folks like you, who are willing to contribute to the JRuby implementation. ❤️ |
@flavorjones code is ready for a review. What is the best way to test this? Are there instructions on how to build JRuby locally? |
@johnnyshields Existing "how to contribute" docs are at https://github.com/sparklemotion/nokogiri/blob/main/CONTRIBUTING.md but the TLDR is |
@flavorjones This is ready for final review. While testing I found two separate issue unrelated to this PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough test coverage! ❤️
@flavorjones all comments are addressed, please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, will merge if the test suite passes.
Will get this into a bugfix release soon. But first I'd like to see if I can easily fix the other bugs we're finding and get them in, too. |
@flavorjones awesome, I really appreciate it! |
@johnnyshields I really appreciate you digging in and fixing this! |
Side note: I'm integration testing Nokogiri Should I be testing ruby-saml with JRuby as well, do you think? You're obviously finding edge cases that nobody else has in 15+ years so it might be worthwhile ... |
@flavorjones yes, please test JRuby and TruffleRuby for RubySaml if possible 🙏 |
See #3465 for CI coverage |
Great work here! |
…rt v1.18x) (#3476) **What problem is this PR intended to solve?** Fix MRI Ruby vs. JRuby XML child namespace output differences Fixes #3455 Backport of #3456 cc @johnnyshields
Fixes #3455