refactor(c14n): extract shared namespace-declaration pipeline into a common helper

[polaz]

Summary

• Extract duplicated namespace-declaration pipeline from InclusiveNsRenderer and ExclusiveNsRenderer into a shared collect_ns_declarations() helper in new ns_common module
• Both renderers now delegate to the helper with a mode-specific predicate (accept-all vs visibly-utilized)
• Removes ~90 lines of duplicated filter/sort/render logic, reducing drift risk between the two implementations

Technical Details

The shared helper owns the full pipeline: xml: prefix filtering → mode predicate → redundant declaration suppression (parent_rendered) → spurious xmlns="" handling → prefix sorting → rendered map update.

The predicate is accepted as impl Fn(&str, &str) -> bool, enabling monomorphization and inlining — no dynamic dispatch overhead per namespace binding.

The only mode-specific logic remains local to each renderer:

• Inclusive: |_, _| true (all in-scope bindings)
• Exclusive: utilized.contains(prefix) || inclusive_prefixes.contains(prefix)

Known Limitations

• No behavioral changes — pure refactoring with identical output

Test Plan

• All 99 existing tests pass (68 unit + 17 c14n integration + 11 uri integration + 3 doctest)
• 6 new unit tests for ns_common covering: xml prefix exclusion, sorting, redundant suppression, spurious xmlns="" suppression, undeclaration emission, predicate filtering
• The node_set subset path (where has_in_scope_default_namespace forces xmlns="" emission when output ancestors are excluded) is already covered by two pre-existing integration tests: subset_xmlns_empty_with_excluded_default_ns_ancestor and subset_no_spurious_xmlns_empty in tests/c14n_integration.rs
• Clippy clean with -D warnings

Closes #7

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cb275e61-fd2b-44d4-a8ec-300fc0ad26a2

📥 Commits

Reviewing files that changed from the base of the PR and between 9de2389 and 13c97ef.

📒 Files selected for processing (3)

• src/c14n/ns_common.rs
• src/c14n/ns_exclusive.rs
• src/c14n/ns_inclusive.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/c14n/ns_exclusive.rs

---

📝 Walkthrough

Walkthrough

Extracted shared namespace-declaration collection logic into a new internal helper collect_ns_declarations, and updated Inclusive/Exclusive C14N renderers to delegate to it; added unit tests and registered the helper module.

Changes

Cohort / File(s)	Summary
Module registration src/c14n/mod.rs	Added mod ns_common; to expose the new internal helper module to the crate.
Shared namespace pipeline src/c14n/ns_common.rs	New module providing pub(crate) fn collect_ns_declarations(...) that gathers in-scope namespace bindings, applies a caller predicate, suppresses redundant or spurious undeclarations, sorts ("" first, then lexicographic), returns declarations and updated rendered map; includes unit tests.
Exclusive C14N src/c14n/ns_exclusive.rs	Replaced manual per-element namespace collection/filter/sort with a call to collect_ns_declarations using a predicate that accepts visibly-utilized prefixes and forced InclusiveNamespaces prefixes.
Inclusive C14N src/c14n/ns_inclusive.rs	Replaced manual per-element namespace collection/filter/sort with `collect_ns_declarations(node, parent_rendered,

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat(c14n): implement XML canonicalization (inclusive + exclusive) #6: Earlier work introducing the namespace-rendering logic in inclusive/exclusive C14N renderers that this change consolidates.

Poem

🐰 I hopped through nodes and prefixes bright,

Collected namespaces by moonlit night;
No duplicates left, no xml spun,
One helper now, the work is done—
A tidy braid of code, hop, delight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: extracting duplicated namespace-declaration pipeline logic into a shared helper function.
Linked Issues check	✅ Passed	The PR fully implements all requirements from issue #7: extraction of shared pipeline, predicate-based filtering, redundant suppression, xmlns handling, sorting, and both renderers delegating to the helper.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to extracting and refactoring the namespace-declaration pipeline; no unrelated modifications are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/#7-refactorc14n-extract-shared-namespace-declaration

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Refactors the C14N namespace rendering implementation by extracting the shared namespace-declaration pipeline (filter/suppress/sort/update) into a common helper, so inclusive/exclusive renderers only supply mode-specific selection logic.

Changes:

• Added ns_common::collect_ns_declarations() to centralize namespace declaration collection/suppression/sorting and rendered-map updates.
• Updated InclusiveNsRenderer and ExclusiveNsRenderer to delegate to the shared helper with mode-specific predicates.
• Wired the new module into src/c14n/mod.rs and added focused unit tests for the shared pipeline.

Reviewed changes

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

File	Description
src/c14n/ns_inclusive.rs	Replaces the inline inclusive namespace pipeline with a call to collect_ns_declarations using an accept-all predicate.
src/c14n/ns_exclusive.rs	Replaces the inline exclusive namespace pipeline with a call to collect_ns_declarations using a visibly-utilized/forced-prefix predicate.
src/c14n/ns_common.rs	Introduces the shared pipeline helper and adds unit tests for filtering/suppression/sorting behavior.
src/c14n/mod.rs	Registers the new ns_common module in the C14N module tree.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[coderabbitai]

🧹 Nitpick comments (1)

src/c14n/ns_common.rs (1)

95-167: Consider a node_set regression too.

The new cases cover full-document paths, but the subtle branch here is the xmlns="" case when output ancestors are omitted from the subset. A focused test through canonicalize's node_set path would pin the behavior has_in_scope_default_namespace(node) is protecting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/c14n/ns_common.rs` around lines 95 - 167, Tests exercise
collect_ns_declarations but miss the canonicalize node_set path where omission
of ancestor nodes can change handling of xmlns=""; add a focused test that calls
canonicalize with a node_set (not full-document) to reproduce the regression and
ensure has_in_scope_default_namespace(node) still forces emitting xmlns="" when
an ancestor default-namespace is not present in the node_set; specifically,
write a test that builds a document with a default namespace on the root and a
child that undeclares it (xmlns=""), then call canonicalize with node_set
containing only the child and assert the output includes xmlns="" (or that
canonicalize emits the undeclaration), referencing canonicalize, node_set, and
has_in_scope_default_namespace to locate the affected code paths.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/c14n/ns_common.rs`:
- Around line 95-167: Tests exercise collect_ns_declarations but miss the
canonicalize node_set path where omission of ancestor nodes can change handling
of xmlns=""; add a focused test that calls canonicalize with a node_set (not
full-document) to reproduce the regression and ensure
has_in_scope_default_namespace(node) still forces emitting xmlns="" when an
ancestor default-namespace is not present in the node_set; specifically, write a
test that builds a document with a default namespace on the root and a child
that undeclares it (xmlns=""), then call canonicalize with node_set containing
only the child and assert the output includes xmlns="" (or that canonicalize
emits the undeclaration), referencing canonicalize, node_set, and
has_in_scope_default_namespace to locate the affected code paths.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cfd69caa-8ef5-4723-bc6e-5b8aaba1196a

📥 Commits

Reviewing files that changed from the base of the PR and between b040599 and 9de2389.

📒 Files selected for processing (4)

• src/c14n/mod.rs
• src/c14n/ns_common.rs
• src/c14n/ns_exclusive.rs
• src/c14n/ns_inclusive.rs

[Copilot]

Pull request overview

Refactors the XML C14N namespace-declaration rendering logic by extracting the shared “collect/filter/suppress/sort/update” pipeline into a new common helper, reducing duplication between inclusive and exclusive namespace renderers.

Changes:

• Added ns_common::collect_ns_declarations() to own the shared namespace-declaration pipeline.
• Updated InclusiveNsRenderer and ExclusiveNsRenderer to delegate to the shared helper with mode-specific predicates.
• Introduced focused unit tests for the shared helper covering key pipeline behaviors.

Reviewed changes

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

File	Description
src/c14n/ns_inclusive.rs	Replaces inline pipeline with a call into the shared helper using an accept-all predicate.
src/c14n/ns_exclusive.rs	Replaces inline pipeline with a call into the shared helper using the visibly-utilized / forced-prefix predicate.
src/c14n/ns_common.rs	New shared helper implementing the namespace-declaration pipeline plus new unit tests.
src/c14n/mod.rs	Registers the new ns_common module.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[polaz]

Re: CodeRabbit nitpick — node_set regression test for xmlns="" with excluded ancestors

This path is already covered by two pre-existing integration tests in tests/c14n_integration.rs:

• subset_xmlns_empty_with_excluded_default_ns_ancestor (line 357) — root declares xmlns="http://example.com", child has xmlns="", node_set includes only child → asserts xmlns="" is emitted (exercises has_in_scope_default_namespace through collect_ns_declarations)
• subset_no_spurious_xmlns_empty (line 391) — no default ns in scope, node_set includes only child → asserts xmlns="" is NOT emitted

Both tests pass through the refactored collect_ns_declarations helper, confirming no regression. Updated PR description to document this coverage.