feat!: Implement `Display`, `PartialEq<...Buf>` and `FromStr` for basic name types #150

lukasfri · 2025-12-27T10:39:45Z

No description provided.

Copilot

Pull request overview

This pull request implements the Display trait for all *Buf (owned) name types and adds cross-type PartialEq implementations to allow direct comparison between borrowed and owned variants. Additionally, it updates the Display implementation for ExpandedName to include the namespace when present, and adds constructor methods for XmlNamespaceBuf.

Key changes:

Updated ExpandedName::Display to format namespace and local name together when namespace is present
Added Display implementations for all *Buf types (delegating to their borrowed counterparts)
Added symmetric PartialEq implementations between borrowed and owned types for QName, XmlNamespace, Prefix, and LocalName
Added new and new_unchecked constructor methods for XmlNamespaceBuf

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-27T10:42:30Z

xmlity/src/lib.rs

+        if let Some(namespace) = &self.namespace {
+            write!(f, "{}:{}", namespace, self.local_name)


The test at line 849 expects expanded_name.to_string() to equal just local_name_text even when a namespace is present (see case at line 843). With the updated Display implementation that includes the namespace, this test will fail for the with_namespace case. The test assertion needs to be updated to match the expected Display format, which should account for whether a namespace is present or not.

Copilot · 2025-12-27T10:42:31Z

xmlity/src/lib.rs

    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.local_name.fmt(f)
+        if let Some(namespace) = &self.namespace {
+            write!(f, "{}:{}", namespace, self.local_name)


Using a colon separator between namespace and local name is problematic because it conflicts with the syntax for qualified names (QName), where the colon separates a prefix from a local name. An ExpandedName represents a namespace URI (not a prefix) combined with a local name. Consider using Clark notation format instead, which uses braces: {namespace}localName. This is a well-established convention for representing expanded names in XML processing.

Suggested change

write!(f, "{}:{}", namespace, self.local_name)

write!(f, "{{{}}}{}", namespace, self.local_name)

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-27T18:21:52Z

xmlity/src/lib.rs

+impl Display for ExpandedNameBuf {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.as_ref().fmt(f)
+    }
+}


The PR adds PartialEq cross-implementations between borrowed and owned versions for all other name types (QName/QNameBuf, XmlNamespace/XmlNamespaceBuf, Prefix/PrefixBuf, LocalName/LocalNameBuf), but these implementations are missing for ExpandedName and ExpandedNameBuf. For consistency, consider adding impl PartialEq<ExpandedName<'_>> for ExpandedNameBuf and impl PartialEq<ExpandedNameBuf> for ExpandedName<'_> following the same pattern as the other types.

Copilot · 2025-12-27T18:21:52Z

xmlity/src/lib.rs

+impl FromStr for ExpandedNameBuf {
+    type Err = ExpandedNameParseError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let s = s.trim();


The use of trim() here is inconsistent with other FromStr implementations in this file (e.g., QNameBuf::from_str at line 324, LocalNameBuf::from_str at line 789, PrefixBuf::from_str at line 624), which do not trim whitespace. This could lead to unexpected behavior where whitespace is silently removed. Consider removing the trim call for consistency, or if trimming is intentional, apply it consistently across all name type parsers.

Suggested change

let s = s.trim();

Copilot · 2025-12-27T18:21:53Z

xmlity/src/lib.rs

    }
 }

+/// An error that can occur when parsing a [`QName`].


The documentation comment incorrectly states this is "An error that can occur when parsing a [QName]", but this enum is actually ExpandedNameParseError and should reference ExpandedName or ExpandedNameBuf instead.

Suggested change

/// An error that can occur when parsing a [`QName`].

/// An error that can occur when parsing an [`ExpandedNameBuf`].

Copilot · 2025-12-27T18:21:53Z

xmlity/src/lib.rs

+impl FromStr for ExpandedNameBuf {
+    type Err = ExpandedNameParseError;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let s = s.trim();
+        if !s.starts_with('{') {
+            let local_name = s.parse()?;
+
+            return Ok(ExpandedNameBuf::new(local_name, None));
+        }
+
+        let closing_brace_index = s
+            .find('}')
+            .ok_or(ExpandedNameParseError::MissingClosingBrace)?;
+        let namespace = s[1..closing_brace_index].parse()?;
+        let local_name = s[(closing_brace_index + 1)..].parse()?;
+        Ok(ExpandedNameBuf::new(local_name, Some(namespace)))
+    }
+}


The FromStr implementation for ExpandedNameBuf lacks test coverage. Other similar types in the file (e.g., QNameBuf at line 893, LocalNameBuf at line 859) have corresponding tests for their FromStr implementations. Consider adding test cases that verify parsing of expanded names with and without namespaces, as well as error cases like missing closing braces or invalid local names.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

xmlity/src/lib.rs:345

After trimming the input at line 337, the prefix and local_name parts extracted from splitting at line 338 will be passed to their respective FromStr implementations (lines 343, 345), which will trim them again (lines 638, 803). This means that a QName like "prefix : localName" (with spaces around the colon) will be accepted and parsed as "prefix:localName", which may not be the intended behavior since XML QNames should not have whitespace around the colon separator. Consider whether the trimming should only happen at the top level or if the individual parts should be trimmed.

        let s = s.trim();
        let (prefix, local_name) = s.split_once(':').unwrap_or(("", s));

        let prefix = if prefix.is_empty() {
            None
        } else {
            Some(PrefixBuf::from_str(prefix)?)
        };
        let local_name = LocalNameBuf::from_str(local_name)?;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-27T18:41:31Z

xmlity/src/lib.rs

+        let s = s.trim();
+        if !s.starts_with('{') {
+            let local_name = s.parse()?;
+
+            return Ok(ExpandedNameBuf::new(local_name, None));
+        }
+
+        let closing_brace_index = s
+            .find('}')
+            .ok_or(ExpandedNameParseError::MissingClosingBrace)?;
+        let namespace = s[1..closing_brace_index].parse()?;
+        let local_name = s[(closing_brace_index + 1)..].parse()?;
+        Ok(ExpandedNameBuf::new(local_name, Some(namespace)))


Similar to the QNameBuf parser, the trimming at line 219 combined with the recursive trimming in the LocalNameBuf and XmlNamespaceBuf parsers (lines 491, 803) could lead to accepting inputs with internal whitespace. For example, "{http://example.com} localName" (with a space after the closing brace) would be parsed successfully, which may not be the intended behavior for expanded names. Consider whether trimming should only occur at the top level or if the format should be strictly validated.

Copilot · 2025-12-27T18:41:31Z

xmlity/src/lib.rs

+    #[error("Missing closing brace in expanded name")]
+    MissingClosingBrace,


The new ExpandedNameParseError::MissingClosingBrace error case is not covered by tests. Consider adding a test case that verifies the error is correctly returned when parsing an expanded name like "{http://example.com" (missing closing brace).

Copilot · 2025-12-27T18:41:32Z

xmlity/src/lib.rs

+impl PartialEq<ExpandedName<'_>> for ExpandedNameBuf {
+    fn eq(&self, other: &ExpandedName<'_>) -> bool {
+        self.as_ref() == *other
+    }
+}
+
+impl PartialEq<ExpandedNameBuf> for ExpandedName<'_> {
+    fn eq(&self, other: &ExpandedNameBuf) -> bool {
+        *self == other.as_ref()
+    }
+}


The new PartialEq implementations between owned (*Buf) and borrowed types (e.g., ExpandedNameBuf and ExpandedName, QNameBuf and QName, etc.) are not covered by tests. Consider adding tests that verify these cross-type comparisons work correctly.

Copilot · 2025-12-27T18:41:32Z

xmlity/src/lib.rs

+impl Display for ExpandedNameBuf {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.as_ref().fmt(f)
+    }
+}


The new Display implementations for the owned types (ExpandedNameBuf, XmlNamespaceBuf, PrefixBuf, LocalNameBuf) are not directly tested. While the borrowed types' Display implementations are tested indirectly through to_string() calls, consider adding explicit tests for the owned types to ensure the delegation works correctly.

feat: Implement Display and PartialEq<...Buf> for basic name types

2e61c23

Copilot AI review requested due to automatic review settings December 27, 2025 10:39

Copilot started reviewing on behalf of lukasfri December 27, 2025 10:40 View session

Fix test.

7eb6fe9

Copilot AI reviewed Dec 27, 2025

View reviewed changes

Changed display format to use Clark notation. Thanks Copilot?

324576d

lukasfri changed the title ~~feat: Implement Display and PartialEq<...Buf> for basic name types~~ feat: Implement Display, PartialEq<...Buf> and FromStr for basic name types Dec 27, 2025

Adds FromStr to ExpandedName.

e18a728

lukasfri changed the title ~~feat: Implement Display, PartialEq<...Buf> and FromStr for basic name types~~ feat!: Implement Display, PartialEq<...Buf> and FromStr for basic name types Dec 27, 2025

Fixed missing closing brace error.

c0944b9

lukasfri requested a review from Copilot December 27, 2025 18:17

Copilot started reviewing on behalf of lukasfri December 27, 2025 18:18 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

Adds universal trimming. Adds test for ExpandedName parsing.

1bd1da7

lukasfri requested a review from Copilot December 27, 2025 18:38

Copilot started reviewing on behalf of lukasfri December 27, 2025 18:38 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

lukasfri merged commit 0958e8a into main Dec 27, 2025
12 checks passed

lukasfri deleted the feat/trait-impls branch December 27, 2025 18:59

github-actions bot mentioned this pull request Dec 27, 2025

chore: release v0.0.10 #151

Open

		if let Some(namespace) = &self.namespace {
		write!(f, "{}:{}", namespace, self.local_name)

	write!(f, "{}:{}", namespace, self.local_name)
	write!(f, "{{{}}}{}", namespace, self.local_name)

	/// An error that can occur when parsing a [`QName`].
	/// An error that can occur when parsing an [`ExpandedNameBuf`].

		#[error("Missing closing brace in expanded name")]
		MissingClosingBrace,

feat!: Implement Display, PartialEq<...Buf> and FromStr for basic name types #150

feat!: Implement Display, PartialEq<...Buf> and FromStr for basic name types #150

Uh oh!

Conversation

lukasfri commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat!: Implement `Display`, `PartialEq<...Buf>` and `FromStr` for basic name types #150

feat!: Implement `Display`, `PartialEq<...Buf>` and `FromStr` for basic name types #150