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

Fix Invalid Comment Edge Case #441

Merged
merged 1 commit into from
Dec 5, 2020

Conversation

isomarcte
Copy link
Contributor

@isomarcte isomarcte commented Sep 6, 2020

According to the XML specification XML comments can not end in --->. Prior to this commit creating a comment of the form Comment("invalid-") would yield no error though it should, since it is rendered as <!--invalid--->. This commit makes such String values yield an IllegalArgumentException.

A companion object was also added with two helper methods, validated and validatedOpt, which allow for a user of the library to check if a String is valid without having to catch the IllegalArgumentException.

@isomarcte
Copy link
Contributor Author

I had to have the Comment companion object extend scala.runtime.AbstractFunction1 in order to not break binary compatibility.

I'm also fine changing that to any of the following,

  • Filter the binary compatibility issue since it is unlikely anyone is depending on that.
  • Bumping the version.
  • Removing the companion object and the Either/Option based validation code. Though this is my least preferred option. As a user of this library, I'd personally prefer to have the validation code. That being said, I'm happy to defer to the maintainers' preference.

@isomarcte isomarcte force-pushed the fix-comment-edge-case branch 2 times, most recently from a76cdfc to 4ff40ea Compare September 6, 2020 22:50
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for that patch!

I took a quick look around the codebase, and there are many other constructors that perform validation. I think adding the new validation methods only for Comment and leaving everything else makes the library less consistent. So my preference would be to only add the check in the constructor, but no new public API. Once it's there it has to stay for bin compat...

According to [the XML specification](https://www.w3.org/TR/xml11//#IDA5CES "W3 XML") XML comments can not end in `--->`. Prior to this commit creating a comment of the form `Comment("invalid-")` would yield no error though it should, since it is rendered as `<!--invalid--->`. This commit makes such `String` values yield an `IllegalArgumentException`.

A companion object was also added with two helper methods, `validated` and `validatedOpt`, which allow for a user of the library to check if a `String` is valid without having to `catch` the `IllegalArgumentException`.
@isomarcte isomarcte force-pushed the fix-comment-edge-case branch from 4ff40ea to 74778d6 Compare September 7, 2020 20:03
@isomarcte
Copy link
Contributor Author

@lrytz I've updated the PR and removed the companion object.

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SethTisue SethTisue merged commit 13c3e29 into scala:master Dec 5, 2020
@isomarcte isomarcte deleted the fix-comment-edge-case branch December 6, 2020 19:21
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.

3 participants