Uh oh!
There was an error while loading. Please reload this page.
Simplify element.attributes structure - #335
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
add1797 to
fd3aedbComparefd3aedb to
42e3adfCompare| attr = attr[ @element.prefix ] | ||
| end | ||
| attr | ||
| fetch(name, nil) || attlist_attributes&.[](name) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
ee199a0CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ruby/rexml#335 stopped accepting local name.
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