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

Reimplement subject indicators using :has selectors #61

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jupenur
Copy link
Contributor

@jupenur jupenur commented Aug 22, 2017

Current subject indicator implementation is very buggy, see e.g. #60. This rewrites queries that include subject indicators so that :has selectors are used instead.

So e.g. the AST for:

!* :expression

...is transformed into the equivalent of:

*:has(:expression)

...right after parsing.

Also adds support for :root, :scope, and relative selectors; improves the implementation of :has. E.g. this now works:

:has( > Identifier)

@jupenur
Copy link
Contributor Author

jupenur commented Aug 23, 2017

Oh and this also fixes #25, obviously.

@phenomnomnominal
Copy link

I'd love to see this in TSQuery, and we're depending on ESQuery for the parsing. We could clone this PR and move it into our repo, but would prefer to see this happen this side if possible!

@michaelficarra
Copy link
Member

This PR introduces :root and :scope selectors but adds no tests for them. Please add tests for :root and :scope.

@phenomnomnominal
Copy link

@michaelficarra any chance of progressing this now that tests have been added?

@RoystonS
Copy link

Could we progress this? The documentation links suggest that :has( > Identifier) works, but it doesn't, and this PR fixes that.

@run1t
Copy link

run1t commented Apr 10, 2019

@michaelficarra Hi, can you give us some insight on when someone can take a look at this PR ?
Thanks

@phenomnomnominal
Copy link

Hey @run1t, I now have review powers in this repo, and a bit of time to try to get this through. Are you still keen to move it forward? If so I'll properly check out the changes.

@run1t
Copy link

run1t commented Jul 1, 2019

Hi @phenomnomnominal ,
Yes I am ! It would be very nice :)

/*
* Clones a selector AST
*/
function clone(selector) {

Choose a reason for hiding this comment

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

This whole chunk from here down seems to exist purely to translate the old query syntax into the new. Is that necessary? Is the alternative just a breaking change? I think I'd prefer the latter instead of making the PR huge.

"nested descendant subject": function () {
var matches = esquery(nestedFunctions, "!:function :function AssignmentExpression");
assert.contains([ nestedFunctions.body[0] ], matches);
assert.isSame(1, matches.length);

Choose a reason for hiding this comment

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

Seems like we need more than one new test to cover the changes here?

@@ -33,13 +33,27 @@ selectors = s:selector ss:(_ "," _ selector)* {
return [s].concat(ss.map(function (s) { return s[3]; }));

Choose a reason for hiding this comment

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

If we get rid of the old syntax entirely and break the selector API, can some of the grammar be removed?

@phenomnomnominal
Copy link

Not sure why my review didn't come through about a month ago, I musn't have hit the final button. Either way @run1t, care to check out my review and respond? I'll admit I'm not massively familiar with the changes.

@run1t
Copy link

run1t commented Jul 30, 2019

Hi,
Sorry that I wasn’t able to respond.
I had a lot to do in the last month, I sure be able to check that next week :)
It’s been a long time since I made the changes so I have to go back in the code ^^’

papandreou added a commit to assetgraph/assetgraph that referenced this pull request Sep 1, 2019
@tosmolka
Copy link

Hi, any progress here, is this blocked on something?

We are missing relative selectors (e.g. :has( > Identifier)) in ESLint and believe this would fix that.

Anything we can help with? Thanks.

@michaelficarra
Copy link
Member

@tosmolka The PR author hasn't addressed @phenomnomnominal's review comments. And it looks like this PR needs a rebase. If you'd like to open another PR with these commits, rebase them, and address the comments, we can do another review and possibly merge.

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.

6 participants