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

Conversation

Projects
None yet
2 participants
@regebro
Contributor

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.

@regebro regebro force-pushed the regebro:master branch 3 times, most recently from ce18cfa to 6f9e371 Jun 11, 2018

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

This comment has been minimized.

Member

scoder commented Jun 11, 2018

Tests are missing, should probably go into test_sax.py

@regebro regebro force-pushed the regebro:master branch from 6f9e371 to 2b32974 Jun 12, 2018

Show resolved Hide resolved src/lxml/sax.py Outdated
Show resolved Hide resolved src/lxml/sax.py Outdated

@regebro regebro force-pushed the regebro:master branch from 2b32974 to 18d7a6d Oct 17, 2018

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

@regebro regebro force-pushed the regebro:master branch from 18d7a6d to 035d48a Oct 17, 2018

@scoder
Member

scoder left a comment

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

Show resolved Hide resolved 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

@regebro regebro force-pushed the regebro:master branch from d9179a0 to 8c8e613 Nov 23, 2018

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
@regebro

This comment has been minimized.

Contributor

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

This comment has been minimized.

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.

scoder and others added some commits Nov 23, 2018

@scoder
Member

scoder left a comment

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)

This comment has been minimized.

@scoder

scoder Nov 27, 2018

Member

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

scoder added a commit that referenced this pull request Dec 2, 2018

@scoder

This comment has been minimized.

Member

scoder commented Dec 2, 2018

Thanks for your sustained work on this.

@regebro

This comment has been minimized.

Contributor

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