Repository navigation
fix(soup): support the CSS attribute operators in selectors - #182
Conversation
There was a problem hiding this comment.
Verified bb456f3 locally against base 46a0426. Scope: the soup module only.
What I ran
pytest soup/test_soup_correctness.py: 171 passed at head, 142 at base (matches the PR body).- Differential sweep against the reference: 922 selectors x 3 fixtures, head vs base vs beautifulsoup4 4.14.3 / soupsieve 3.0.3. Zero cases where base agreed with the reference and head disagreed, so the fix introduces no regression. 2238 base-vs-head differences, all of them the intended one, with the dropped constraint no longer widening the match.
- Empty-value semantics for the five operators: head agrees with soupsieve on all 15 attribute/operator combinations, including
|=matching an empty attribute value while~=,^=,$=and*=never match an empty one. - The tello shape: on a fixture with that structure,
form[action*="process_login"] button[type="submit"]returns the same set as the degradedform button[type="submit"]at base (the silent widening), and the reference-matching subset at head. My fixture is smaller than the live page, so the numbers are 4 and 3, not 7 and 3. - Mutation battery over the new tests, 9 mutations, one at a time: 8 are caught (
*=as exact equality 6 failed, class list joined without spaces 2, removing the loud failure 3,|=hyphen branch 1,~=as a substring test 1,^=/$=swapped 8, dropping the operators 18). The 9th is the=list branch, see the inline note. manifest.json: the soupcontent_hashmatches a recomputation from the file, andzerodep version-check --strictexits 0 with every module up to date. A full regeneration here toucheslast_updatedonly: 9 unrelated modules move from...Zto+00:00(the PR body says eight) and soup's ownlast_updatedstays at the old2026-04-27value, so the committed file is the partial regeneration the body describes. Nothing reads that field for correctness. The generator emitting the local UTC offset (-05:00here,+00:00on a CI runner) is the churn the body flags as a separate tooling change.
CI
Lint is green at bb456f3. The three red test jobs fail only in toon/, and only because the reference is installed unpinned from git: with toon_format 1.1.0 from git main I get the same 5 failures on base and head (5 failed, 110 passed on both), and the toon/ blobs are identical between base and head. That is #183, not this PR.
Verdict
The operator implementations match the reference and the behaviour change is the fix rather than a reshuffle of the bug. I found no blocking issue, and I am not approving yet only because the merge checklist wants a green test job and it is red for #183. Once #183 lands, rebase this branch and I will approve at the new head. The inline notes are non-blocking.
One small nit not worth its own thread: the _ATTR_OPERATORS membership check in _parse_attr_token and the trailing raise ValueError in _attr_operator_matches are both unreachable (mutating either leaves the suite green at 171). Both already carry # pragma: no cover, so this is only about whether the dead branches earn their keep.
Consumer side, checked directly rather than taken from the FWIW
Oaklight/Tello-Renewal vendors soup 0.6.0 and calls select/select_one in 8 places: 6 in src/tello_renewal/web/html_parser.py (span.card_text > span, .balance-details, div.subtitle > div.subtitle_heading, div.subtitle > div.subtitle_name, button#renew_plan, .pack_card) and two constants in tests/test_login_flow.py (#account_login_form button[type="submit"], #form_consent button[value="accept_all"]). Every one of those uses tag, class, id, child/descendant, or the [attr="value"] form the parser already read, so no selector there can reach the new ValueError. The remaining bracket selectors in that repo (input[name="username"], input[name="products[mobile_pack][product_id]"]) are handed to Playwright, not to soup.
| """ | ||
| if op == "=": | ||
| if isinstance(actual, list): | ||
| return expected in actual |
There was a problem hiding this comment.
nit: this is the one branch that still disagrees with the reference, and no test pins it.
Changing this branch to the soupsieve/spec behaviour (_attr_text(actual) == expected, comparing the serialized attribute instead of list membership) leaves all 171 tests green. So the suite does not guard it either way, and it is also where the module still differs from beautifulsoup4: on the PR's own fixture, [class="btn"] matches 2 tags here against 0 in soupsieve, and [class="btn primary"] matches 0 against 1.
A [class="..."] row in the matrix would pin the current behaviour and make that known difference visible to whoever reads the matrix next. Not a request to change the behaviour in this PR.
There was a problem hiding this comment.
Changed it rather than pinned it — you and Elena measured the same branch from opposite sides, and the same conclusion came out of both.
= now compares against the serialisation (_attr_text(actual) == expected), so it agrees with ~=/*= and with soupsieve.
Four rows pin it: button[class="btn"], button[class="btn primary"], button[class="btn secondary"] and button[class="btn"][type="submit"]. Reverting the comparison to membership fails exactly those four and nothing else, so the branch is guarded now, and the 9th mutation in your battery is caught.
There was a problem hiding this comment.
Reviewed the diff and the reference matrix, and re-ran the operator set plus ~40 extra selectors against soupsieve. The operator implementations themselves look right: *=, ^=, $=, ~=, |= now agree with the reference in both the ancestor and target position, including the empty-value cases (~=, ^=, $=, *= never matching an empty expected) and the list-valued class serialisation. The ValueError behaviour change is reasonable and well-documented, and select's contract is clearer for it.
One substantive gap inline, plus two nits.
Nit 1 — _ATTR_OPERATORS is dead code. (?P<op>[~|^$*]?=) can only ever produce the six operators in the set, so if op not in _ATTR_OPERATORS cannot fire. The # pragma: no cover is honest about that, but a guard that can never guard is better either kept purely as documentation or dropped — as written it reads like a runtime check.
Nit 2 — the new ValueError also fires on valid CSS. _SIMPLE_RE still tokenises [...] as \[[^\]]+\], so a ] inside a quoted value ends the token early: [title="a]b"], which soupsieve matches, now raises unsupported attribute selector. Raising beats the previous silent widening, so this is an improvement rather than a regression — but it means the message says "unsupported" for syntax that is merely untokenisable by this parser. One line in the note would let a vendoring caller tell the two apart.
Verified locally in the zerodep env (Python 3.11.17, bs4 4.14.3, soupsieve 2.9.2): 171 passed in soup/test_soup_correctness.py, and the downstream readability module (165 passed) is unaffected. The consumer-side claim also holds — tello-renewal's parsers use only tag, .class, #id and child/descendant combinators, so none of them can reach the new ValueError path.
| """ | ||
| if op == "=": | ||
| if isinstance(actual, list): | ||
| return expected in actual |
There was a problem hiding this comment.
This = branch is where the threat this PR fixes is still open, for the list-valued class attribute. _attr_text() was added to serialise the list for the substring operators, but = keeps membership semantics, so the comparison runs against the class list rather than against the attribute string.
Measured on this PR's own ATTR_OPERATOR_HTML, ours vs soupsieve 2.9.2 / bs4 4.14.3:
button[class="btn"] ours ['A', 'B'] soupsieve []
button[class="btn primary"] ours [] soupsieve ['A']
button[class="btn"][type="submit"] ours ['B'] soupsieve []
The third row is the same silent widening this PR exists to remove — reached through = instead of *=. return _attr_text(actual) == expected would make = agree with ~=/*= and with the reference.
This is pre-existing rather than introduced here, so it is a scope call, not a blocker. Flagging it because this PR rewrites this exact function, documents [attr="value"] as supported, and adopts soupsieve as the reference — and per the comment on the test matrix, that combination is the one the new tests cannot see.
There was a problem hiding this comment.
Implemented. = now compares _attr_text(actual) == expected, so it agrees with ~=/*= and with the reference.
Your three rows on ATTR_OPERATOR_HTML, before and after, against soupsieve 2.10:
| selector | before | after | soupsieve |
|---|---|---|---|
button[class="btn"] |
['A', 'B'] |
[] |
[] |
button[class="btn primary"] |
[] |
['A'] |
['A'] |
button[class="btn"][type="submit"] |
['B'] |
[] |
[] |
plus button[class="btn secondary"], which was [] and is now ['B'] against ['B'].
All four are matrix rows now. Reverting the comparison to membership fails exactly those four, so the branch is guarded in both directions.
| the compound silently degraded to its tag name. | ||
| """ | ||
|
|
||
| OPERATOR_SELECTORS = [ |
There was a problem hiding this comment.
The matrix spells = only on plain-string attributes (form[action="x"], form[id="form_b"], button[type="submit"]), so the list-valued class attribute is the one combination the operator matrix never exercises — which is exactly where = still diverges from soupsieve (see the comment on _attr_operator_matches). 'button[class="btn"]' and 'button[class="btn primary"]' on this fixture would pin it, and they fail against the reference today.
There was a problem hiding this comment.
Added, one more than you asked for. The matrix now carries button[class="btn"], button[class="btn primary"], button[class="btn secondary"] and button[class="btn"][type="submit"].
The compound one is your third row from the other thread: it reaches the same silent widening through = instead of *=, which is why I wanted it in the suite and not only the two simple forms.
Suite is 175 passed. Mutating = back to membership fails exactly these four rows, so the new coverage is load-bearing rather than decorative.
3b2ca85 to
4dbe0c5
Compare
|
Body updated for the two review-body items that had no inline thread, plus the numbers that changed.
Suite is 175 passed, was 171. Mutating |
`[attr*="value"]`, `[attr^="value"]`, `[attr$="value"]` and the other operators were tokenized as attribute constraints but never matched `_ATTR_RE`, which understands only `[attr]` and `[attr="value"]`. The token was dropped, so the compound degraded to its tag name and the match widened silently. On the Tello login page that made `form[action*="process_login"] button[type="submit"]` match all seven submit buttons instead of the three in forms posting to `process_login`. Implement `~=`, `|=`, `^=`, `$=` and `*=`, joining the list-valued `class` attribute with spaces for the substring forms, and raise `ValueError` for attribute syntax the parser still cannot read so an unsupported selector fails loudly instead of widening. Closes #181
`_attr_operator_matches` applied `=` to the list-valued `class` by membership, so `button[class="btn"]` matched `class="btn primary"` and `button[class="btn primary"]` matched nothing. `_attr_text()` already serialised the list for the substring operators; `=` now compares against that serialisation, which is what soupsieve does. That membership test was the same silent widening this PR exists to remove, reached through `=` instead of `*=`: `button[class="btn"][type="submit"]` matched a tag whose class list does not serialise to `btn`. The operator matrix spelled `=` only on plain-string attributes, the one combination that could not see this. Four `class` rows pin it now, and reverting the comparison to membership fails exactly those four.
The `Raises:` note and the error message carried examples, while the set itself is the compatibility contract for a caller that vendors this module: each of these selectors used to widen the match silently and now raises. Checked against soupsieve 2.10, the set splits three ways. - Accepted by the reference, not implemented here: the Selectors L4 case-sensitivity flags `[attr="v" i]` and `[attr="v" s]`, the namespace forms `[svg|use]`, `[*|href]`, `[|href]`, and the jQuery-style `[attr!="x"]`. The PR body named only `[attr!=x]`, `[=x]` and unterminated quotes. - Valid CSS this parser cannot tokenise: a `]` inside a quoted value, as in `[title="a]b"]`. `_SIMPLE_RE` matches `\[[^\]]+\]`, so the token is cut short. The message now separates that from syntax the parser declines, so a caller can tell the two apart. - Rejected by the reference too, so consistent with it: `[=x]`, unterminated quotes, and `[xlink:href]`. Drops the `_ATTR_OPERATORS` membership test and the constant: `(?P<op>[~|^$*]?=)` cannot produce anything outside the set, so the check could not fire and read like a runtime guard. `_attr_operator_matches` is the single dispatch point now, and its trailing raise is what catches an operator the regex learns without a branch.
The soup tests compare selectors against soupsieve, which is what actually evaluates them, and `bench-soup` pinned only beautifulsoup4. The parity baseline therefore floated, and not evenly: soupsieve 3.0.x requires >=3.11.5, so CI resolved 3.0.3 on 3.11 and up and 2.10 on 3.10. A 3.12 run and a 3.10 run were checking the module against different references. 2.10 is the newest release that installs across the whole matrix, so all four jobs compare against one baseline. Raise it deliberately, with a run of the soup suite, the way the pinned toon reference is raised.
…t entry HTML matches the legacy attribute set (`type`, `lang`, `dir`, `rel`, `method` and the rest) ASCII case-insensitively and soupsieve implements that, so a selector that differs only in case misses matches the reference finds. Verified against soupsieve 2.10 on a fixture holding `type="submit-alpha"`, `type="SUBMIT"` and `type="submit"`: `[type^="SUB"]` returns 1 here against 3 there, `[type*="UBM"]` 1 against 3, `[type$="A"]` 0 against 1, and each result is a subset of the reference's. The divergence is inherited, and it always narrows the match rather than widening it, unlike the dropped-constraint bug this PR fixes. Not fixed here: honouring the legacy set is a behaviour change of its own and needs the attribute list from the HTML spec plus its own sweep, not the operator matrix. soup/soup.py changes here, so the manifest's soup `content_hash` and `last_updated` are refreshed from the generator's own helpers (`_content_hash`, `_git_last_updated`) -- the values a regeneration writes for this module, which is why `last_updated` leaves `2026-04-27`. The other modules are left alone: the generator emitting the local UTC offset is the separate tooling change behind that churn.
4dbe0c5 to
291042d
Compare
|
Rebased onto master ( The three red test jobs were stale, not soup. They came from run
One conflict, in Locally at the rebased head: What each of you raised, and where it is:
Your inline comments all have replies. Nothing is waiting on me — a re-review at |
There was a problem hiding this comment.
Re-checked at 291042d against the pinned baseline (beautifulsoup4==4.14.3, soupsieve==2.10), independently of the green CI.
My original finding is fixed. I re-ran my operator matrix on this PR's own ATTR_OPERATOR_HTML fixture. On the pre-PR base it produced 13 value differences across 14 selectors; on this head, 0.
| selector | base | head | soupsieve 2.10 |
|---|---|---|---|
form[action*="process_login"] button[type="submit"] |
['B','C'] |
['B'] |
['B'] |
button[class="btn"] |
['A','B'] |
[] |
[] |
button[class="btn primary"] |
[] |
['A'] |
['A'] |
button[class="btn"][type="submit"] |
['B'] |
[] |
[] |
form[action^="/account"] |
['A','B','C'] |
['B'] |
['B'] |
So = compares the exact attribute form against the serialised value, as 01e0518 says. Both of my inline comments are addressed — the first one was that divergence, the second was that the matrix never exercised = on the list-valued class, and it now does.
Suite at the pin: pytest soup/test_soup_correctness.py → 175 passed.
Case-insensitivity, checked in the safe direction. I tested set inclusion rather than eyeballing it: [type="SUBMIT"], [type="Submit"] and [TYPE="submit"] each return a subset of what the reference returns, and no case widens. Agreed with your reading — it cannot manufacture a match — and #187 tracks it.
One gap that sits outside the Raises: taxonomy (filed as #191, non-blocking). The raise-instead-of-drop contract holds for a single attribute selector but not for the selector-list form. A quoted comma splits the list, and the result is an empty list with no exception, discarding groups that parse correctly:
| selector | ours | soupsieve 2.10 |
|---|---|---|
[title="a,b"] |
['t2'] |
['t2'] |
[title="a,b"], [title="c"] |
[] |
['t2','t3'] |
[title="c"], [title="a,b"] |
[] |
['t2','t3'] |
The single-selector case parses fine, so the attribute parser is not at fault; the group separator is split without regard for quotes. Identical at the base commit, so it is pre-existing rather than a regression, and it under-matches rather than widening — not a blocker here. But it is the last place a constraint can still disappear silently, which is the property this PR exists to establish, so it is worth an issue rather than a comment.
Approving.
There was a problem hiding this comment.
Re-checked at 291042d (base 6ac319a). This supersedes my review at bb456f3: the branch was force-pushed as a rebase, that head is no longer an ancestor, so none of the earlier probes carry over and everything below was re-run.
Verified on this head, soupsieve 2.10 (the new pin) / beautifulsoup4 4.14.3
- Suite: 175 passed (142 at master, 171 at my reviewed head).
- Differential sweep, 936 selectors x 3 fixtures, head vs master vs the reference: 0 regressions. 15 divergences remain, all of them the
typecase family plus the pre-existing[data-UPPER]attribute-name case. Every one under-matches, so none can manufacture a match. The limit is now recorded and #187 tracks it. - The
=fix is real and now pinned:[class="btn"]->['1']and[class="btn primary"]->['2', '3']on my fixture, both equal to the reference, where master returned['1', '2', '3']and[]. Reverting the branch to list membership fails 4 tests. - Empty-value semantics agree with the reference on all 15 attribute/operator combinations, including
|=matching an empty attribute value while~=,^=,$=and*=never match. - The docstring taxonomy matches actual behaviour. Each of the 10 claimed selectors behaves as documented, and the accept/reject split against the reference is right in every case:
[type!="x"],[type="x" i],[type="x" s],[svg|use],[*|href],[|href],[title="a]b"]raise here and are accepted there;[=x], unterminated quotes and[xlink:href]are rejected by both. - Mutation battery, 10 mutations one at a time: all 10 caught (drop the operators 18 failed,
*=as exact equality 6, class joined without spaces 4, loud failure removed 3,|=hyphen branch 1,~=as substring 1,^=/$=swapped 8,=back to list membership 4,_unquoteno-op 23). The dead_ATTR_OPERATORSguard is gone too. - Manifest:
content_hashmatches a recomputation,last_updatedequals what_git_last_updatedderives at this head (2026-10-13T03:17:08-05:00),zerodep version-check --strictexits 0. - The pin is effective: CI installs
soupsieve==2.10, and the 3.10 job now reports 7544 passed against 7546 on 3.11/3.12/3.13, up from the 2638 it collected when the extras floated. A green 3.10 now covers the reference comparisons.
One non-blocking gap, recorded here rather than as an inline thread so it does not gate the merge:
The empty-value rule documented in _attr_operator_matches ("~=, ^=, $= and *= never match an empty expected") is not pinned by any test. Removing the bool(expected) guard from any of the five operators leaves the suite green at 175, checked one at a time. For ^=, $= and *= that guard is load-bearing in exactly the direction this PR exists to prevent: dropped, [class^=""] stops meaning "no match" and starts matching every element carrying the attribute. |='s empty-value match is unpinned the same way. A parametrized row would close it ([attr~=""], [attr^=""], [attr$=""], [attr*=""] -> no match, [attr|=""] -> matches an empty attribute value). Behaviour today is correct against the reference in all 15 combinations, so this is coverage, not a defect, and it should not hold the merge. I will file it as a follow-up issue if you would rather not touch this branch again; say the word and I will.
Elena's open thread on soup/test_soup_correctness.py:393 is functionally covered by the class-= rows added in this branch. I resolved my own three threads after verifying each fix at this head.
Approving at 291042d: lint and the 3.10-3.13 test jobs are green, the PR is CLEAN and MERGEABLE, the delta shows no regression against the reference, and every finding from my previous review is addressed and now guarded by a test.
What
select()accepted any[...]token but only understood[attr]and[attr="value"]. Every other operator was tokenized, failed to parse as a constraint, and was dropped, so the compound degraded to its tag name and the match widened silently. Fixes #181.soup/soup.py—_ATTR_RElearns the operator (~=,|=,^=,$=,*=);_parse_attr_tokenreturns(name, operator, value)and the matcher applies the operator. The list-valuedclassattribute is serialised with spaces for every operator including=, which is what soupsieve does. Attribute syntax the parser still cannot read now raisesValueErrorinstead of dropping the constraint, which is the behaviour change in this PR.soup/test_soup_correctness.py— a discriminating three-form fixture, the operator matrix compared against beautifulsoup4, a pin for the widening itself, and the loud-failure cases. 175 tests pass in the module (was 142).pyproject.toml—bench-souppins soupsieve, the library that actually evaluates the selectors.manifest.json— soup entry refreshed for the 0.7.0 bump, then kept matching the file.Evidence
Same fixture and selectors, soup on the left, soupsieve on the right:
form[action*="process_login"] button['A', 'B', 'C']['B']['B']form[action^="/account"] button['A', 'B', 'C']['B']['B']form[action$="process_login"] button['A', 'B', 'C']['B']['B']button[type*="alpha"]['A', 'B', 'C']['A']['A']button[type$="alpha"]['A', 'B', 'C']['A']['A']button[type~="submit-alpha"]['A', 'B', 'C']['A']['A']form[id*="form_"] button['A', 'B', 'C']['A', 'B']['A', 'B']button[type|="submit-"]['A', 'B', 'C'][][]The
|=row is why the test matrix carriesbutton[type|="submit-"]: on the plain[type|="submit"]spelling the dropped constraint happens to agree with soupsieve, so that spelling alone would not pin the operator.Twenty-eight selectors in the matrix agree with beautifulsoup4, and the previously-working forms (
[attr],[attr="value"], descendant, child,:not) are in the same list so a regression there fails the same test.On the real Tello login page (the shape from Oaklight/Tello-Renewal#10),
form[action*="process_login"] button[type="submit"]now returns 3, matching Chromium, where it returned 7 before.Mutation-checked the new tests, one mutation at a time, 175 passed at baseline: dropping the operators again fails 14 tests; making
*=exact equality, joining the class list without spaces, and removing the loud failure each fail the tests written for them; and reverting=to list membership fails exactly the fourclassrows added in review.Review round
The non-blocking findings from both reviewers became three commits on top of the original head.
=against the list-valuedclass(Elena, Milo). The branch applied membership, sobutton[class="btn"]matchedclass="btn primary"andbutton[class="btn primary"]matched nothing. That is the same silent widening this PR removes, reached through=instead of*=.=now compares the serialised attribute:button[class="btn"]['A', 'B'][][]button[class="btn primary"][]['A']['A']button[class="btn"][type="submit"]['B'][][]button[class="btn secondary"][]['B']['B']The matrix spelled
=only on plain-string attributes, the one combination that could not see this, so those four rows are in it now.The set of syntax that newly raises (Milo, Elena). Checked against soupsieve 2.10, it splits three ways, and the
Raises:docstring and the error message carry all three instead of examples:[attr="v" i],[attr="v" s],[svg|use],[*|href],[|href], and[attr!="x"]. The body had grouped!=with[=x], but soupsieve accepts!=; we raise.[title="a]b"][=x], unterminated quotes,[xlink:href]_ATTR_OPERATORS(Elena, Milo). Dropped the membership check and the set itself:(?P<op>[~|^$*]?=)cannot produce anything outside it, so the check could not fire._attr_operator_matchesis the single dispatch point now, and its trailing raise is what catches an operator the regex learns without a branch.soupsieve pinned (Milo). Not to 3.0.3: 3.0.x requires
>=3.11.5, so that pin would drop soupsieve out of the 3.10 job entirely and the soup tests would skip there. It also means the baseline was not uniform before — 3.11 and up resolved 3.0.3 while 3.10 resolved 2.10, so two matrix entries were checking different references.bench-souppins2.10, the newest release that installs across the whole matrix.Two things are deliberately not fixed here, and both are recorded: the case-sensitivity gap for the legacy attribute set (#187), and
_SIMPLE_RE's inability to read a]inside a quoted value.Checks
ruff check,ruff format,ty checkandcomplexipy -mx 45all pass viapre-commit run --all-files;zerodep version-check --strictreports all modules up to date;zerodep dep-check souppasses soup and its only downstream dependent, readability.Notes
make manifeston this machine rewriteslast_updatedfrom...Zto...+00:00for nine unrelated modules, and soup's own entry was left behind by the partial regeneration. The soup entry now carries what the generator's own helpers produce for it (_content_hash,_git_last_updated). The other nine are untouched: the generator emitting the local UTC offset is the tooling change behind that churn, not this PR.Behaviour change: selectors using attribute syntax this parser cannot read now raise
ValueErrorinstead of returning a widened match set. The full set is in theRaises:docstring ofselect.AI was used to assist with the investigation and the patch.