{{ message }}
Conversation
The 'qname' argument is not only used for element names, so calling the non-namespace part 'tag' is confusing. The XML namespaces spec calls it 'local part', so I picked 'local' as the new name.
All other callers of add_qname() already check the argument type prior to calling, so by also checking before adding an attribute name, add_qname() doesn't need to deal with TypeError anymore. The diff is actually tiny when whitespace changes are ignored.
This is in preparation of a future change that requires the default namespace to not be applied to attributes. The cache population code will become non-trivial then.
This is in preparation of a future change that requires the default namespace to not be applied to attributes.
Currently there is no real need to do this, since the serialization used for tags and attributes is identical. But in the future, they will use different serializations and we want to avoid defining namespace prefixes for namespaces that are not going to appear in the serialization.
This is in preparation of a future change that requires the default namespace to not be applied to attributes. Because the default namespace is no longer in the mapping, the first generated synthetic namespace prefix is now always "ns0", while previously it was "ns0" or "ns1" depending on whether a default namespace was provided. I updated the expected serialization in one test case to match this new behavior.
Previously, it returned a mapping from namespace to prefix. But later the serialization code sorted that mapping by prefix, so inverting the mapping simplifies things. Another reason to invert the mapping is that currently there is a 1:1 correspondence between prefixes and namespaces, but in the future it will be possible for the default namespace to exist both with an empty prefix and with a actual prefix.
This is not my work: I extracted this test case from the second iteration of wiml's patch for issue 17088. https://bugs.python.org/file33125/bug17088_2.patch I did modify the line wrapping, as the original patch went very wide (over column 100). I also updated the prefix numbering in the expected serialized output, since numbering always starts at 0 now.
Unprefixed attributes are considered to not be in any namespace, according to the XML namespaces spec. This also fixes issue 17088, since the exception that rejects non-qualified names when using the default_namespace option is no longer raised when serializing an attribute name.
When no default_namespace is passed, serialize_qname() returns the same value regardless of whether is_attr is True or False. So we can save some time by storing the serialization for both the tag and attribute in the cache when one of them is computed.
scoder
reviewed
Apr 4, 2020
Comment on lines
-871
to
-872
| except TypeError: | ||
| _raise_serialization_error(qname) |
Contributor
There was a problem hiding this comment.
Where's this part gone? Wouldn't this leak a TypeError to the user side?
| add_qname(tag.text) | ||
| add_qname(tag.text) | ||
| elif isinstance(tag, str): | ||
| if tag not in qnames: |
Contributor
There was a problem hiding this comment.
These conditions might have been there for performance reasons. Not sure if it matters, but that's up to some benchmarking I think.
| add_qname(text.text) | ||
| return qnames, namespaces | ||
|
|
||
| prefix_map = {prefix: ns for ns, prefix in namespaces.items()} |
Contributor
There was a problem hiding this comment.
Prefixes don't have to be globally unique, so this might introduce bugs. (Suggests to me that there might be missing tests.)
Comment on lines
+879
to
+880
| if not default_namespace: | ||
| ser_tag = ser_attr |
Contributor
There was a problem hiding this comment.
This seems worth a comment in the code (likewise below).
Contributor
|
@mthuurne, please take a look at the code review. Thanks! |
|
This PR is stale because it has been open for 30 days with no activity. |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

I split the changes over several commits for easier reviewing. Feel free to squash some or all of them later.
I took the approach that Stefan Behnel suggested in the bug discussion: change the
qnamescache values from a single serialized value shared by tag and attribute to a pair that contains a separate serialized value for tag and attribute. Often the two will be identical, but unqualified names interact differently with the default namespace depending on whether it's a tag name or an attribute name.Many thanks to "wiml" for the detailed test case.
https://bugs.python.org/issue17088