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

Adds NsReader::prefixes to iterate on all the prefixes currently declared #705

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Jan 14, 2024

This is useful for some use cases like RDF/XML where prefixes might be interesting to extract

@Tpt
Copy link
Contributor Author

Tpt commented Jan 14, 2024

Sorry for the out-of-the-blue PR. Don't feel bad for rejecting/requesting big changes. Thank you so much for quick-xml

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83fc2d6) 65.39% compared to head (adf873e) 65.46%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   65.39%   65.46%   +0.07%     
==========================================
  Files          38       38              
  Lines       18128    18165      +37     
==========================================
+ Hits        11854    11892      +38     
+ Misses       6274     6273       -1     
Flag Coverage Δ
unittests 65.46% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

This is nice addition!

Could you add changelog entry and a doctest to better explain the results of the new method. There is also a couple of questions that I left in the code.

src/reader/ns_reader.rs Outdated Show resolved Hide resolved
tests/namespaces.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
@Mingun Mingun added enhancement namespaces Issues related to namespaces support labels Jan 14, 2024
Copy link
Contributor Author

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Thank you for your so quick code review. I have made the changes you suggested

src/name.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
tests/namespaces.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Mingun Mingun left a comment

Choose a reason for hiding this comment

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

Thanks! A minor fix in changelog is required and it seems to me a small optimization is possible

Changelog.md Outdated
@@ -29,6 +29,7 @@ to get an offset of the error position. For `SyntaxError`s the range
- [#362]: Added `escape::minimal_escape()` which escapes only `&` and `<`.
- [#362]: Added `BytesCData::minimal_escape()` which escapes only `&` and `<`.
- [#362]: Added `Serializer::set_quote_level()` which allow to set desired level of escaping.
- `NsReader::prefixes` to list all the prefixes currently declared.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `NsReader::prefixes` to list all the prefixes currently declared.
- [#705]: Added `NsReader::prefixes()` to list all the prefixes currently declared.

and add link to this PR at line 70

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/name.rs Outdated
Comment on lines 646 to 657
if self.resolver.bindings[self.bindings_cursor..]
.iter()
.any(|ne| {
namespace_entry.prefix(&self.resolver.buffer)
== ne.prefix(&self.resolver.buffer)
})
{
continue; // Overriden
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we can return None here because on all next iterations we also enter in this if, don't we?

Also, namespace_entry.prefix(&self.resolver.buffer) could be calculated outside of closure and I have a feeling that the whole this loop can be embedded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, namespace_entry.prefix(&self.resolver.buffer) could be calculated outside of closure and I have a feeling that the whole this loop can be embedded.

Indeed. Done. Thank you!

It seems we can return None here because on all next iterations we also enter in this if, don't we?

I don't think so because we check only if a namespace of the same name is declared, not if an other namespace in general is declared.

Let's say we have

<foo:outer xmlns:foo="a" xmlns:bar="b">
  <foo:inner xmlns:foo="c"></foo:inner>
</foo:outer>

The bindings list when parsing foo:inner is going to be something like [("foo", "a"), ("bar", "b"), ("foo", "c")] so, when we iterate on it we don't want to stop iterating on the first ("foo", "a") because it has been overriden by ("foo", "c").

src/name.rs Outdated
}

fn size_hint(&self) -> (usize, Option<usize>) {
(0, Some(self.resolver.bindings.len() - 2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(0, Some(self.resolver.bindings.len() - 2))
// -2 for xml and xmlns prefixes that are always stored in bindings
(0, Some(self.resolver.bindings.len() - 2))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Tpt
Copy link
Contributor Author

Tpt commented Jan 15, 2024

Thanks! A minor fix in changelog is required and it seems to me a small optimization is possible

Thank you! Done.

…ared

This is useful for some use cases like RDF/XML where prefixes might be interesting to extract
@Mingun
Copy link
Collaborator

Mingun commented Jan 21, 2024

I've made returned iterator Cloneable and Debugable and slightly tune the size_hint method to return more close upper bound and test it. Implementing ExactSizeIterator is unreasonably although, because this will require to go through self.resolver.bindings to count exact length.

Thanks for your work, @Tpt!

@Mingun Mingun merged commit 88aa477 into tafia:master Jan 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement namespaces Issues related to namespaces support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants