Skip to content

Simplify element.attributes structure#335

Merged
naitoh merged 3 commits into
ruby:masterfrom
tompng:attlist_reference_refactor
Jul 5, 2026
Merged

Simplify element.attributes structure#335
naitoh merged 3 commits into
ruby:masterfrom
tompng:attlist_reference_refactor

Conversation

@tompng

@tompng tompng commented Jun 15, 2026

Copy link
Copy Markdown
Member

Basically refactor Attributes, and fixes some bugs found while doing it.

Structure of Attributes, a subclass of Hash

For XML: <element name1="1" ns1:name2="2" ns2:name2="3" ns1:name3="4"/>, the structure was:

{
  name1 => attr1
  name2 => { ns1 => attr2, ns2 => attr3 }
  name3 => att4
}

This structure is useful for fuzzy name search, and fuzzy name deletion, but we don't need this complicated structure because fuzzy search should be removed, and fuzzy deletion is not used from REXML (and it wasn't working at all)

I think the mental model of namespaced attribute in Document Object Model is something like this.

{ default: { name1: attr1 }, ns1: { name2: attr1, name3: attr4}, ns2: { name2: attr2 } }
# or simply
{ "name1": attr1, "ns1:name2": attr2, "ns2: name2": attr3, "ns1:name3": attr4 }

This PR changes it to the latter, simple one.

Stop fuzzy get_attribute/get_attribute_ns search

Attributes#get_attribute does fuzzy search only on its own attribute, not on attributes defined in ATTLIST. Attributes#get_attribute_ns tries fuzzy search but fails because of a bug.
In Document Object Model, get_attribute and get_attribute_ns should perform strictly no-prefix attribute if prefix/namespace is not given.

ATTLIST lookup

There's many redundant duplicated code for ATTLIST lookup.
Attributes#get_attribute_ns forgot to check ATTLIST (bug).
Introduce each_effective_attribute that iterates attributes defined in ATTLIST and element's own attributes

Fixed bugs

  • get_attribute: Fuzzy search shouldn't be performed
  • get_attribute_ns: forgot to lookup ATTLIST
  • delete_all: Unlike the name that indicates fuzzy deletion, it deletes only one attribute, and returns wrong attrribute which is a different one than actually deleted when prefix is given.
  • namespace declaration check: foo:xmnls was wrongly considered namespace declaration
  • And perhaps more which I haven't found yet

Copilot AI review requested due to automatic review settings June 15, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors REXML attribute handling to simplify lookup/storage while reintroducing ATTLIST (DTD) default attribute support, and updates/extends tests to match the new behavior.

Changes:

  • Simplify attribute storage/access in REXML::Element::Attributes, adding support utilities for ATTLIST defaults and namespace-declaration detection.
  • Add Doctype#attribute_declarations_of and refactor existing DTD attribute accessors to use it.
  • Update and add tests around attribute deletion, namespace-qualified lookups, and attributes.to_h expectations.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/test_core.rb Updates expectations for attribute hashing and adds backward-compat coverage for DTD attribute accessors.
test/test_contrib.rb Adjusts tests to use fully qualified attribute names for namespaced attributes.
test/test_attributes.rb Adds coverage for delete_all behavior across namespaces.
lib/rexml/element.rb Refactors attribute iteration/lookup/deletion and introduces “effective attributes” merging element + DTD defaults.
lib/rexml/doctype.rb Adds attribute_declarations_of and refactors attributes_of / attribute_of to use it.
lib/rexml/attribute.rb Changes removal semantics and adds namespace_declaration? helper.

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

Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/doctype.rb
Comment thread lib/rexml/element.rb
@tompng tompng force-pushed the attlist_reference_refactor branch from add1797 to fd3aedb Compare June 16, 2026 05:38
Copilot AI review requested due to automatic review settings June 16, 2026 18:28
@tompng tompng force-pushed the attlist_reference_refactor branch from fd3aedb to 42e3adf Compare June 16, 2026 18:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment thread lib/rexml/element.rb
attr = attr[ @element.prefix ]
end
attr
fetch(name, nil) || attlist_attributes&.[](name)
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb
Change REXML::Attributes internal structure, Refactor ATTLIST handling in Attributes.
Fix inconsistent beavior of handling ATTLIST.
Stop fuzzy get_attribute/get_attribute_ns search and make it Document Object Model complient.
@tompng tompng force-pushed the attlist_reference_refactor branch from 42e3adf to ee199a0 Compare July 3, 2026 17:43
Copilot AI review requested due to automatic review settings July 4, 2026 15:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Comment thread lib/rexml/attribute.rb Outdated
Comment thread lib/rexml/element.rb
Comment thread lib/rexml/element.rb

@naitoh naitoh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this PR is reasonable corrections.
Thank you!

@naitoh naitoh merged commit 6a0255c into ruby:master Jul 5, 2026
66 of 73 checks passed
@tompng tompng deleted the attlist_reference_refactor branch July 5, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants