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: fix conditional type parsing association #253

Closed
wants to merge 1 commit into from

Conversation

HerringtonDarkholme
Copy link
Contributor

@HerringtonDarkholme HerringtonDarkholme commented Jul 14, 2023

fix #231

I updated the test corpus manually since it looks like most of the corpus is not generated by tree-sitter test -u.

Using update snapshot caused a lot of changes so I didn't do it for better review readability.

I can do a separate PR to update the snapshot by automatic generation.

Checklist:

  • All tests pass in CI.
  • 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).

@amaanq
Copy link
Member

amaanq commented Jul 14, 2023

Not sure what you mean by update snapshot, if you mean the changes from tree-sitter test -u, on my side I see no additional changes

@HerringtonDarkholme
Copy link
Contributor Author

@amaanq this is because no new change is updated. Try adding a new test case and rerunning tree-sitter test -u.

I reused the term snapshot test from jest to describe the action of updating the test corpus. I would expect that file is semi-manually updated by human? The s-expression part is generated by the machine and the code part is written by humans.

@amaanq
Copy link
Member

amaanq commented Jul 14, 2023

Oh, if you mean you refrained from adding more tests to avoid polluting the diff, don't worry! The more the better (within reason)

@HerringtonDarkholme
Copy link
Contributor Author

Oh, I can show you the change I see locally.

This line is what I actually added.
image

But unrelated test cases are impacted.
image

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

I see, I can reproduce that after all - that should be fixed in ts core, but for now if you wouldn't mind updating the test manually that'd be appreciated

Side note, do you use ahlinc's tree-sitter fork for better cli output? It's a bit behind master so generation output is almost not the same, causing CI to fail, use upstream for that or I can force push for you (I had the same experience myself)

@HerringtonDarkholme
Copy link
Contributor Author

@amaanq thanks for your suggestion! I am running npm run build but it fails to generate the expected output somehow.

If you can force push for me that will be much appreciated!

@amaanq
Copy link
Member

amaanq commented Jul 15, 2023

Unfortunately I cannot push to your branch, this is probably because it's under the "ast-grep" org, or because you did not allow repo maintainers to edit your pr, or because the branch name is master (for PRs don't use master/main as it causes issues when checking out)

@HerringtonDarkholme
Copy link
Contributor Author

Let me change it, thanks!

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.

conditional_type should use prec.right instead of prec.left.
2 participants