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

Call xmlSubstituteEntitiesDefault before parsing #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samoconnor
Copy link

Given that LightXML does not support XML Entities, asking LibXML to expand entities while parsing seems like the right thing to do...

@samoconnor
Copy link
Author

bump

@@ -85,12 +85,14 @@ end
#### parse and free

function parse_file(filename::AbstractString)
ccall((:xmlSubstituteEntitiesDefault, libxml2), Cint, (Cint,), Int(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

@compat Int(1)

Copy link
Contributor

Choose a reason for hiding this comment

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

or, just 1

@samoconnor
Copy link
Author

bump again...

@nalimilan
Copy link
Member

Could you squash the two commits into a single one?

Given that LightXML does not support XML Entities, asking LibXML to expand entities while parsing seems like the right thing to do...
@samoconnor
Copy link
Author

rebase/squashed

@nalimilan
Copy link
Member

Looks good to me, but since I know nothing about this package I'll leave @tkelman press the button.

Also, maybe this should be tested? Else it might break without anybody noticing and you'll likely be the first victim.

@samoconnor
Copy link
Author

I'll leave it to @tkelman to decide if he thinks this change breaks anything or if he want's to add more tests.
The tests over at XMLDict.jl are reasonably thorough. They include transforming the whole "REC-xml-20081126.xml" standard into a Dict then transforming back to XML. Please feel free to reuse these tests in LightXML if you want...

@nalimilan
Copy link
Member

The tests over at XMLDict.jl are reasonably thorough. They include transforming the whole "REC-xml-20081126.xml" standard into a Dict then transforming back to XML. Please feel free to reuse these tests in LightXML if you want...

I'm not particularly interested in this, but if you want to be sure this continues working, you'd better move some of them here. Else you'll only notice the breakage after a LightXML release has been tagged.

@tkelman
Copy link
Contributor

tkelman commented Mar 10, 2016

What does this accomplish, why is it necessary, what are the impacts? If there's a nonzero cost to doing this by default so you may have a reason to want to maintain the existing behavior, maybe a boolean keyword argument to enable or disable this?

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