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

Let ElementTreeProducer use the available namespaces #267

Merged
merged 6 commits into from
Dec 2, 2018
Merged

Let ElementTreeProducer use the available namespaces #267

merged 6 commits into from
Dec 2, 2018

Conversation

regebro
Copy link
Contributor

@regebro regebro commented Jun 11, 2018

sax.ElementTreeProducer would ignore the namespace prefixes that were available in the element tree, and always generate new prefixes like ns00, ns01 etc.

This PR uses the prefixes in the XML by looking at the elements nsmap.

src/lxml/sax.py Outdated Show resolved Hide resolved
@scoder
Copy link
Member

scoder commented Jun 11, 2018

Tests are missing, should probably go into test_sax.py

src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
ElementTreeProducer would ignore the namespace prefixes that were available in the element tree, and always generate new prefixes like ns00, ns01 etc.
Copy link
Member

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I think preferring the current prefix is a good idea. I left comments on the rest.

src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
CHANGES.txt Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
src/lxml/sax.py Outdated Show resolved Hide resolved
@regebro
Copy link
Contributor Author

regebro commented Nov 23, 2018

I'm not sure what happened with the tests here. I did, after much trouble replicate this locally, but I have no clue what is going on.

@scoder
Copy link
Member

scoder commented Nov 23, 2018

I think that's the latest libxml2 release that strikes here, 2.9.9-rc1. I reported the problem on their side and configured the travis build (in the master branch) to use the previous release, which seems to work correctly. Rebasing on master should fix this. Sorry for the inconvenience.

Copy link
Member

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Nice. Looks much better now!

continue
break
prefix = min(pfx for (pfx, uri) in nsmap.items()
if pfx is not None and uri == ns_uri)
Copy link
Member

Choose a reason for hiding this comment

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

One more thing to think about: this will fail now if there is only the default prefix or if the URI does not have a prefix. (min() requires at least one value.) The second case probably shouldn't happen (and could be considered an actual error case), but the first seems ok and shouldn't fail.

@scoder scoder added the feature label Dec 2, 2018
@scoder scoder merged commit 9ecef44 into lxml:master Dec 2, 2018
scoder added a commit that referenced this pull request Dec 2, 2018
@scoder
Copy link
Member

scoder commented Dec 2, 2018

Thanks for your sustained work on this.

@regebro
Copy link
Contributor Author

regebro commented Dec 3, 2018

No worries, it's been fun. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants