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

Do not ignore XML comments when parsing #508

Closed
dubinsky opened this issue Apr 15, 2021 · 9 comments · Fixed by #549
Closed

Do not ignore XML comments when parsing #508

dubinsky opened this issue Apr 15, 2021 · 9 comments · Fixed by #549

Comments

@dubinsky
Copy link
Contributor

scala-xml uses scala.xml.Comment to represent XML comments if they are present in the XML literals, and allows constructing comment nodes explicitly; when parsing XML not from a literal, scala.xml.factory.XMLLoader does not set
an instance of org.xml.sax.ext.LexicalHandler as an "http://xml.org/sax/properties/lexical-handler" property on the underlying parser, and does not provide any factory methods that such lexical handler could call from its comment() method.

As a result, comments present in the XML being loaded get ignored.

Are there any plans to fix this?

Thanks!

@SethTisue
Copy link
Member

There don’t seem to be any other tickets about this.

In general, there aren't "plans" for scala-xml except to keep on merging volunteer pull requests that come in and keep doing releases.

@dubinsky
Copy link
Contributor Author

... keep on merging volunteer pull requests that come in ...

Here is one then: #549; please take a look ;)

@ashawley
Copy link
Member

I'll take a look. Thanks for contributing this.

@ashawley
Copy link
Member

ashawley commented Aug 19, 2021

Since it's new behavior, I guess this could be part of a 2.1.0 release of scala-xml.

@ashawley
Copy link
Member

ashawley commented Aug 23, 2021

The patch introduces some non-trivial changes to the the library's core parsing infrastructure, but there's no other way to do it. It's worth accepting, IMO. Thanks again for the contribution.

@dubinsky
Copy link
Contributor Author

Since it's new behavior, I guess this could be part of a 2.1.0 release of scala-xml.

Isn't every fixed bug new behavior? :)

@dubinsky
Copy link
Contributor Author

@ashawley

It's worth accepting, IMO. Thanks again for the contribution.

Thank you for your kind words!
I have some follow-up fixes planned (CDATA sections, processing instructions and the like).
Will my pull request be merged so that I can build on it or do you prefer everything in one pull request?
Thanks!

@ashawley
Copy link
Member

Isn't every fixed bug new behavior? :)

True, if fixing comments was a bug fix it would be 2.0.2, but your change is more involved.

Will my pull request be merged so that I can build on it or do you prefer everything in one pull request?

Yes, we'll try to get it merged soon. Then your further fixes can be new pull requests.

@dubinsky
Copy link
Contributor Author

@ashawley

Will my pull request be merged so that I can build on it or do you prefer everything in one pull request?

Yes, we'll try to get it merged soon. Then your further fixes can be new pull requests.

Great, thanks!

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 a pull request may close this issue.

3 participants