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: removed unused keyword #179

Merged
merged 3 commits into from
Jul 5, 2023
Merged

fix: removed unused keyword #179

merged 3 commits into from
Jul 5, 2023

Conversation

DerekStride
Copy link
Owner

What

fixes #155

Use the keyword_language node in the grammar rules.

@DerekStride DerekStride requested review from dmfay and matthias-Q July 4, 2023 15:46
@DerekStride DerekStride self-assigned this Jul 4, 2023
@DerekStride
Copy link
Owner Author

@matthias-Q I'm requesting another review, I added some more commits that ensure our keyword_ nodes are in sync with the highlights.scm file.

You can see the failure message here: https://github.com/DerekStride/tree-sitter-sql/actions/runs/5456777435/jobs/9930044879

And see it fixed in the latest commit.

@DerekStride DerekStride requested a review from matthias-Q July 4, 2023 16:26
@matthias-Q
Copy link
Collaborator

I am bit short on time today. I can take a look tomorrow. I was noticing a lot of "new" keywords in the highlights. Did we missed all of them? Do we need to add them to nvim-treesitter as well? If so, I would maybe put some of them not under the @keyword query.

@DerekStride DerekStride force-pushed the ds/remove-unused-keyword branch from 61f6e9b to f1bdfdf Compare July 4, 2023 17:41
@DerekStride DerekStride force-pushed the ds/remove-unused-keyword branch from f1bdfdf to 27fd9fd Compare July 4, 2023 17:42
@DerekStride
Copy link
Owner Author

Did we missed all of them? Do we need to add them to nvim-treesitter as well?

yeah they were all missing & need to be synced to nvim-treesitter.

I would maybe put some of them not under the @Keyword query.

I moved a few of them to @attribute, there's still a lot of generic @keywords but I'm not too sure where else they could belong

Copy link
Collaborator

@matthias-Q matthias-Q left a comment

Choose a reason for hiding this comment

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

Looks good. I made two suggestions for the highlights, but in the end I expect the maintainers of nvim-treesitter to have their own opinion 😄

@@ -9,7 +9,6 @@
(keyword_spgist)
(keyword_gin)
(keyword_brin)
(keyword_float)
Copy link
Collaborator

Choose a reason for hiding this comment

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

float can be a @function.call and a @type.builtin

select 
 1::float;

select 
 float(1);

So I would keep it there. in nvim-treesitter - the last match wins, thus the order matters here (which makes things unnecessarily complicated imo.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

We already have float in @type.builtin and the rule for invocation should cover the @function.call case.

(keyword_analyze)
(keyword_rewrite)
(keyword_statistics)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think statistics and stats should be @type.qualifier and not @keyword

Copy link
Owner Author

Choose a reason for hiding this comment

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

I removed these from here because they were duplicated in @type.qualifier already 👍

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.

keyword_language is unused, renderet to parser.c as aux_sym_*
2 participants