Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the usage of type selector in :has() #57

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

jbhoot
Copy link
Contributor

@jbhoot jbhoot commented Jul 25, 2024

The current implementation of :has() selector does not exclude text nodes while selecting elements.

Subsequently, during the parsing, these text nodes end up being passed to function name node, which throws an error because it expects only element nodes.

As a result, currently a selector like :has(ul), which contains a type selector as its argument, fails.

This PR fixes the aforementioned bug.

In the process, I also improved the assertion errors in the count tests, which now print, expected: x but got: y, instead of just not equals.

@coveralls
Copy link

coveralls commented Jul 25, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 3733caa on justbhoot:fix-simple-selector-in-has-argument
into e3d6cbb on aantron:master.

Jayesh Bhoot added 3 commits July 25, 2024 13:43
`descendants` function also includes text nodes,
which causes `name node` function to fail down the line,
because it only expects an ``Element of n`, not
``Text of string`, which a text node is.

In any case, :has() doesn't select text nodes. So,
excluding them beforehand is the way to go.
@jbhoot jbhoot force-pushed the fix-simple-selector-in-has-argument branch from df86095 to 596e776 Compare July 25, 2024 08:13
@jbhoot
Copy link
Contributor Author

jbhoot commented Aug 9, 2024

@aantron I don't like that you have to spend time in cleaning up formatting everytime I make a PR.

What formatter do you use? Can you also share your configuration for the same?

@aantron aantron merged commit 6c05f4d into aantron:master Aug 21, 2024
4 checks passed
@aantron
Copy link
Owner

aantron commented Aug 21, 2024

Thank you!

@aantron I don't like that you have to spend time in cleaning up formatting everytime I make a PR.

What formatter do you use? Can you also share your configuration for the same?

I actually prefer to work this way, as it allows us to both work efficiently without unnecessary round-trips on minor matters. As long as you don't mind :)

I don't use a formatter, as the formatters around as of last year made very illogical and diff-unfriendly choices. I opened some issues about that on ocamlformat, and I think there was some progress on this, but I haven't had time to check it yet.

@jbhoot
Copy link
Contributor Author

jbhoot commented Aug 22, 2024

Whatever works for you is good enough for me. Thank you for the merge!

@jbhoot jbhoot deleted the fix-simple-selector-in-has-argument branch August 31, 2024 17:17
This pull request was closed.
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