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 XMLEventReader does not handle ' properly #85

Closed
wants to merge 1 commit into from

Conversation

fehmicansaglam
Copy link
Contributor

@fehmicansaglam fehmicansaglam commented Oct 20, 2015

@SethTisue
Copy link
Member

@fehmicansaglam I'm sorry this seems to have been ignored. it needs a rebase now. are you interested in doing a rebase? perhaps this comment will attract some reviewer attention.

@fehmicansaglam
Copy link
Contributor Author

Sure, I will.

@SethTisue
Copy link
Member

are this and #89 duplicates?

@fehmicansaglam
Copy link
Contributor Author

fehmicansaglam commented Feb 6, 2017

@SethTisue yes, they are duplicates. According to your comment here, #89 is better. If so, I can close this.

@SethTisue
Copy link
Member

@ashawley I'm confused by your comment on #85. can you make the call here?

@ashawley
Copy link
Member

@SethTisue This one is good. I had assumed fixing the XMLEventReader bug would have broader impact. I was incorrect. I eventually cleared up my own confusion in this comment at #72. So PR #89 was a false lead, but also an attempt to understand the issues.

@@ -93,7 +93,7 @@ object Utility extends AnyRef with parsing.TokenTests {
* For reasons unclear escape and unescape are a long ways from
* being logical inverses.
*/
val pairs = Map(
private val pairs = Map(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe annotate it as @deprecated first just in case a user happened to be importing it?

@ashawley
Copy link
Member

Fixed instead by 3b0775b. I used a version of your unit test in 6824df8. With any luck, this will be released in 1.0.7 relatively soon.

Thanks for looking in to this and submitting the PR.

Sorry it took so long to get this merged.

@ashawley ashawley closed this May 24, 2017
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