feat: Changes XML-related types to have owned and borrowed variants instead of being Cows. - #138
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR transitions XML-related types from using Cow-based ownership patterns to having dedicated owned (-Buf) and borrowed variants. This refactoring removes the lifetime parameters from the owned types and introduces a more explicit ownership model.
Key changes:
- Introduces
*Bufvariants (e.g.,LocalNameBuf,XmlNamespaceBuf,PrefixBuf,ExpandedNameBuf,QNameBuf) as owned counterparts to borrowed types - Removes
Cowusage in favor of explicit owned/borrowed distinction - Updates all construction patterns from
new()tofrom_str()for owned variants andnew()for borrowed variants
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| xmlity/src/lib.rs | Core type definitions updated with *Buf variants and ownership model changes |
| xmlity/src/value/mod.rs | Updated XmlElement and XmlAttribute to use *Buf variants |
| xmlity/src/value/serializer.rs | Updated serialization infrastructure to use new type patterns |
| xmlity/src/value/serialize.rs | Updated serialize implementations to use as_ref() for type conversion |
| xmlity/src/value/deserializer.rs | Updated deserialization to use as_ref() for borrowed access |
| xmlity/src/value/deserialize.rs | Updated context traits to use borrowed references |
| xmlity/src/ser.rs | Updated serialization traits to use borrowed prefix references |
| xmlity/src/de.rs | Updated deserialization context traits |
| xmlity/src/noop.rs | Updated no-op serializer signatures |
| xmlity/src/macros.rs | Updated macro generation to use new type construction patterns |
| xmlity/tests/*.rs | Updated test code to use from_str() and *Buf variants |
| xmlity-quick-xml/src/*.rs | Updated quick-xml integration for new type system |
| xmlity-quick-xml/tests/*.rs | Updated integration tests |
| xmlity-quick-xml/examples/*.rs | Updated examples |
| xmlity-derive/src/*.rs | Updated derive macro implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ExpandedName::new( | ||
| self.local_name.into_owned(), | ||
| self.namespace.map(|n| n.into_owned()), | ||
| // /// Converts this [`ExpandedName`] into an owned version. |
There was a problem hiding this comment.
The commented-out documentation line should be uncommented since the method is active and public.
Suggested change
| // /// Converts this [`ExpandedName`] into an owned version. | |
| /// Converts this [`ExpandedName`] into an owned version. |
Comment on lines
129
to
146
| // /// Returns this [`ExpandedName`] as a reference. | ||
| // pub fn as_ref(&self) -> ExpandedName<'_> { | ||
| // ExpandedName::new( | ||
| // self.local_name.as_ref(), | ||
| // self.namespace.as_ref().map(|n| n.as_ref()), | ||
| // ) | ||
| // } | ||
|
|
||
| /// Returns the local name of this [`ExpandedName`]. | ||
| pub fn local_name(&self) -> &LocalName<'a> { | ||
| pub fn local_name(&self) -> &'a LocalName { | ||
| &self.local_name | ||
| } | ||
|
|
||
| /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| pub fn local_name_mut(&mut self) -> &mut LocalName<'a> { | ||
| &mut self.local_name | ||
| } | ||
| // /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| // pub fn local_name_mut(&mut self) -> &'a mut LocalName { | ||
| // &mut self.local_name | ||
| // } | ||
|
|
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. If this functionality might be needed later, consider removing it and relying on version control history.
Comment on lines
129
to
156
| // /// Returns this [`ExpandedName`] as a reference. | ||
| // pub fn as_ref(&self) -> ExpandedName<'_> { | ||
| // ExpandedName::new( | ||
| // self.local_name.as_ref(), | ||
| // self.namespace.as_ref().map(|n| n.as_ref()), | ||
| // ) | ||
| // } | ||
|
|
||
| /// Returns the local name of this [`ExpandedName`]. | ||
| pub fn local_name(&self) -> &LocalName<'a> { | ||
| pub fn local_name(&self) -> &'a LocalName { | ||
| &self.local_name | ||
| } | ||
|
|
||
| /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| pub fn local_name_mut(&mut self) -> &mut LocalName<'a> { | ||
| &mut self.local_name | ||
| } | ||
| // /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| // pub fn local_name_mut(&mut self) -> &'a mut LocalName { | ||
| // &mut self.local_name | ||
| // } | ||
|
|
||
| /// Returns the namespace of this [`ExpandedName`]. | ||
| pub fn namespace(&self) -> &Option<XmlNamespace<'a>> { | ||
| pub fn namespace(&self) -> &Option<&'a XmlNamespace> { | ||
| &self.namespace | ||
| } | ||
|
|
||
| /// Returns a mutable reference to the namespace of this [`ExpandedName`]. | ||
| pub fn namespace_mut(&mut self) -> &mut Option<XmlNamespace<'a>> { | ||
| &mut self.namespace | ||
| } | ||
| // /// Returns a mutable reference to the namespace of this [`ExpandedName`]. | ||
| // pub fn namespace_mut(&mut self) -> &mut Option<XmlNamespace<'a>> { | ||
| // &mut self.namespace | ||
| // } | ||
|
|
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
Comment on lines
129
to
156
| // /// Returns this [`ExpandedName`] as a reference. | ||
| // pub fn as_ref(&self) -> ExpandedName<'_> { | ||
| // ExpandedName::new( | ||
| // self.local_name.as_ref(), | ||
| // self.namespace.as_ref().map(|n| n.as_ref()), | ||
| // ) | ||
| // } | ||
|
|
||
| /// Returns the local name of this [`ExpandedName`]. | ||
| pub fn local_name(&self) -> &LocalName<'a> { | ||
| pub fn local_name(&self) -> &'a LocalName { | ||
| &self.local_name | ||
| } | ||
|
|
||
| /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| pub fn local_name_mut(&mut self) -> &mut LocalName<'a> { | ||
| &mut self.local_name | ||
| } | ||
| // /// Returns a mutable reference to the local name of this [`ExpandedName`]. | ||
| // pub fn local_name_mut(&mut self) -> &'a mut LocalName { | ||
| // &mut self.local_name | ||
| // } | ||
|
|
||
| /// Returns the namespace of this [`ExpandedName`]. | ||
| pub fn namespace(&self) -> &Option<XmlNamespace<'a>> { | ||
| pub fn namespace(&self) -> &Option<&'a XmlNamespace> { | ||
| &self.namespace | ||
| } | ||
|
|
||
| /// Returns a mutable reference to the namespace of this [`ExpandedName`]. | ||
| pub fn namespace_mut(&mut self) -> &mut Option<XmlNamespace<'a>> { | ||
| &mut self.namespace | ||
| } | ||
| // /// Returns a mutable reference to the namespace of this [`ExpandedName`]. | ||
| // pub fn namespace_mut(&mut self) -> &mut Option<XmlNamespace<'a>> { | ||
| // &mut self.namespace | ||
| // } | ||
|
|
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
Comment on lines
368
to
377
| // /// Converts this [`XmlNamespace`] into an owned version. | ||
| // pub fn into_owned(self) -> XmlNamespace<'static> { | ||
| // XmlNamespace(Cow::Owned(self.0.into_owned())) | ||
| // } | ||
|
|
||
| // /// Returns this [`XmlNamespace`] as a reference. | ||
| // pub fn as_ref(&self) -> XmlNamespace<'_> { | ||
| // XmlNamespace(Cow::Borrowed(&self.0)) | ||
| // } | ||
|
|
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
Suggested change
| // /// Converts this [`XmlNamespace`] into an owned version. | |
| // pub fn into_owned(self) -> XmlNamespace<'static> { | |
| // XmlNamespace(Cow::Owned(self.0.into_owned())) | |
| // } | |
| // /// Returns this [`XmlNamespace`] as a reference. | |
| // pub fn as_ref(&self) -> XmlNamespace<'_> { | |
| // XmlNamespace(Cow::Borrowed(&self.0)) | |
| // } |
Comment on lines
492
to
514
| // /// Converts this [`Prefix`] into an owned version. | ||
| // pub fn into_owned(self) -> Prefix<'static> { | ||
| // Prefix(Cow::Owned(self.0.into_owned())) | ||
| // } | ||
|
|
||
| // /// Returns this [`Prefix`] as a reference. | ||
| // pub fn as_ref(&self) -> Prefix<'_> { | ||
| // Prefix(Cow::Borrowed(&self.0)) | ||
| // } | ||
|
|
||
| /// Returns this [`Prefix`] as a string slice. | ||
| pub fn as_str(&self) -> &str { | ||
| &self.0 | ||
| } | ||
|
|
||
| /// Returns this [`Prefix`] as a [`QName`] with the `xmlns` prefix. This is useful for serializing namespaces. | ||
| pub fn xmlns(&'a self) -> QName<'a> { | ||
| QName::new( | ||
| Prefix::new("xmlns").expect("xmlns is a valid prefix"), | ||
| LocalName::from(self.clone()), | ||
| ) | ||
| } | ||
| // /// Returns this [`Prefix`] as a [`QName`] with the `xmlns` prefix. This is useful for serializing namespaces. | ||
| // pub fn xmlns(&'a self) -> QName<'a> { | ||
| // QName::new( | ||
| // Prefix::new("xmlns").expect("xmlns is a valid prefix"), | ||
| // LocalName::from(self.clone()), | ||
| // ) | ||
| // } | ||
|
|
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
Comment on lines
+206
to
+212
| // pub fn resolve_bytes_start<'a>(&'a self, bytes_start: &'a BytesStart<'a>) -> ExpandedName<'a> { | ||
| // self.resolve_qname(bytes_start.name(), false) | ||
| // } | ||
|
|
||
| pub fn resolve_attribute<'a>(&'a self, attribute: &'a Attribute<'a>) -> ExpandedName<'a> { | ||
| self.resolve_qname(attribute.key, true) | ||
| } | ||
| // pub fn resolve_attribute<'a>(&'a self, attribute: &'a Attribute<'a>) -> ExpandedName<'a> { | ||
| // self.resolve_qname(attribute.key, true) | ||
| // } |
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
Comment on lines
+206
to
+212
| // pub fn resolve_bytes_start<'a>(&'a self, bytes_start: &'a BytesStart<'a>) -> ExpandedName<'a> { | ||
| // self.resolve_qname(bytes_start.name(), false) | ||
| // } | ||
|
|
||
| pub fn resolve_attribute<'a>(&'a self, attribute: &'a Attribute<'a>) -> ExpandedName<'a> { | ||
| self.resolve_qname(attribute.key, true) | ||
| } | ||
| // pub fn resolve_attribute<'a>(&'a self, attribute: &'a Attribute<'a>) -> ExpandedName<'a> { | ||
| // self.resolve_qname(attribute.key, true) | ||
| // } |
There was a problem hiding this comment.
Dead code should be removed instead of being commented out. Consider removing commented-out methods to reduce code clutter.
lukasfri
force-pushed
the
feat/owned-borrowed-types
branch
from
December 25, 2025 10:57
cb6b7e4 to
3ca5a91
Compare
…nstead of being `Cow`s.
lukasfri
force-pushed
the
feat/owned-borrowed-types
branch
from
December 25, 2025 11:08
3ca5a91 to
ace7dc6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+327
to
+329
| pub fn new(value: &str) -> Result<&Self, XmlNamespaceParseError> { | ||
| //TODO: Validate URI | ||
| // SAFETY: The value has been validated. |
There was a problem hiding this comment.
The new method returns a Result but never actually returns an error because validation is not implemented (marked with TODO on line 328). The function should either implement proper URI validation or document clearly that validation is not yet implemented and all inputs are currently accepted. The SAFETY comment claiming "The value has been validated" is misleading when validation is not performed.
Suggested change
| pub fn new(value: &str) -> Result<&Self, XmlNamespaceParseError> { | |
| //TODO: Validate URI | |
| // SAFETY: The value has been validated. | |
| /// | |
| /// **Note:** URI validation is not yet implemented. At present, this | |
| /// function accepts any input and always returns `Ok`, and the `Result` | |
| /// return type is reserved for future validation logic. | |
| pub fn new(value: &str) -> Result<&Self, XmlNamespaceParseError> { | |
| // TODO: Implement URI validation. For now, all inputs are accepted. | |
| // SAFETY: This currently calls `new_unchecked`, so correctness relies | |
| // on the caller not relying on URI validation being performed here. |
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.
This changes
LocalName,QName,PrefixandXmlNamespaceto have -Bufvariants with slice types, instead of all beingCow-based. I've wanted to do this for a while, but haven't been bothered to do so until now.