Commit
- Loading branch information
There are no files selected for viewing
9 comments
on commit 795cc5a
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.
Hi @tenderlove, this commit seems to be broken. There's only test cases for replace_entities = true
. You should have also tested the output with replace_entities = false
. You'll notice the result is the same. Could you please make replace_entities = false
work as expected, by sending raw "&" to the sax parser with no substitutions?
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.
@rosenfeld patches welcome
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.
I wish I had good understanding of libxml2 :(
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.
@rosenfeld telling me what "I should have done" on a 7 year old commit isn't a great way to ask for help.
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.
I can contribute with a failing test case though if that helps. Sorry, I didn't mean to take you down. I'm not a native English speaker so please forgive me if something sound rude to you.
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.
I spent a few hours today trying to understand what this replace_entities
really does. After reading lots of PR and issues I ended up watching this commit and decided to comment on what is the root cause of the main problem I'm experiencing and noticed yesterday. I didn't mean to harsh you though.
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.
I even tried to read nokogiri's source and I ran the test suite and changed that param in the test and could confirm the result is the same, that's why I wrote about this, but intending to explain that it doesn't really does what you probably think it does... I tried changing the source to set the default behavior of entity substitutions and the result was the same. I'm quite lost actually, that's why I'm asking for help. Sorry again if this is not what you understood from my previous comments.
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.
Sorry, I didn't mean to take you down. I'm not a native English speaker so please forgive me if something sound rude to you.
No problem. I can understand that! Can you send a failing test case? That would help me debug. It could be that libxml2 is broken and we'll have to fix that.
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.
Sure, I'll work on that after having lunch and create a PR. Thanks a lot!
Try setting this to false and see if the array would be
['a', '&b']
.