Skip to content

Align nested_identifier -> member_expression alias field names #267

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

Closed

Conversation

helixbass
Copy link
Contributor

@helixbass helixbass commented Aug 26, 2023

This PR adds field names to the existing nested_identifier rule (which gets used for dotted JSX element names eg <Foo.Bar />) so that when it gets aliased to $.member_expression it has the same field names as a "normal" member_expression (ie object and property)

This seems desirable to me so that one can treat it as an "invariant" that when you see a member_expression node it definitely has object and property fields

Checklist:

  • All tests pass in CI.
  • The script/parse-examples passes without issues. (I think? I see Successfully parsed 2615 of 2616 example files (100.0%))
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

right: (jsx_element
open_tag: (jsx_opening_element
name: (member_expression
object: (identifier)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw in the docs that you can specify field names in tests so added these here to actually test this change

Then the test was failing unless all field names were specified so replaced the existing test output with what it was now emitting (I assume it's all "correct" including all of the newly-added field names here?)

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's normal - tests w/ fields need all fields added

@amaanq
Copy link
Member

amaanq commented Aug 26, 2023

I'm going to assume you're using the node version of tree-sitter to generate - can you use the Rust crate on master? (notice the small changes made to parser.h, that's bad)

@helixbass
Copy link
Contributor Author

Ok re-ran tree-sitter generate using a locally built version of tree-sitter cli/ with its current master, I see the changes to parser.h went away

@tree-sitter tree-sitter deleted a comment Sep 6, 2023
@amaanq amaanq mentioned this pull request Feb 1, 2024
@amaanq
Copy link
Member

amaanq commented Feb 1, 2024

Hi @helixbass, I cherry picked your changes onto #291, thank you!

@amaanq amaanq closed this Feb 1, 2024
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.

2 participants