Simplify element.attributes structure#335
Merged
Merged
Conversation
There was a problem hiding this comment.
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_ofand refactor existing DTD attribute accessors to use it. - Update and add tests around attribute deletion, namespace-qualified lookups, and
attributes.to_hexpectations.
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.
add1797 to
fd3aedb
Compare
fd3aedb to
42e3adf
Compare
| attr = attr[ @element.prefix ] | ||
| end | ||
| attr | ||
| fetch(name, nil) || attlist_attributes&.[](name) |
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.
42e3adf to
ee199a0
Compare
naitoh
approved these changes
Jul 5, 2026
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.
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: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.
This PR changes it to the latter, simple one.
Stop fuzzy get_attribute/get_attribute_ns search
Attributes#get_attributedoes fuzzy search only on its own attribute, not on attributes defined in ATTLIST.Attributes#get_attribute_nstries 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_nsforgot to check ATTLIST (bug).Introduce
each_effective_attributethat iterates attributes defined in ATTLIST and element's own attributesFixed bugs
foo:xmnlswas wrongly considered namespace declaration