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

feat: allow fixup and amend in subject #48

Merged
merged 1 commit into from
Dec 7, 2023
Merged

feat: allow fixup and amend in subject #48

merged 1 commit into from
Dec 7, 2023

Conversation

gbprod
Copy link
Owner

@gbprod gbprod commented Dec 5, 2023

This will parse git commit subject that are prefixed with fuxup! or amend!.

Ref: #34

@gbprod gbprod force-pushed the feat-fixup-amend branch 7 times, most recently from 9782144 to b84b3dc Compare December 7, 2023 08:48
@gbprod gbprod merged commit 7e3ad5f into main Dec 7, 2023
3 checks passed
@gbprod gbprod deleted the feat-fixup-amend branch December 7, 2023 13:33
@@ -33,6 +33,9 @@ module.exports = grammar({

subject: ($) =>
seq(
optional(
seq(alias(choice('fixup!', 'amend!'), $.subject_prefix), WHITESPACE)
Copy link

@wookayin wookayin Dec 12, 2023

Choose a reason for hiding this comment

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

node_type subject_prefix is undefined. nvim-treesitter/nvim-treesitter#5774 (comment).

We'll also need:

subject_prefix: ($) => ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's an alias, you don't need any rule associated to the name (Ref: https://tree-sitter.github.io/tree-sitter/creating-parsers#the-grammar-dsl)
I'm not sure that it's the origin of your issue, have you updated your parser ? :TSUpdate gitcommit ?

Copy link

@wookayin wookayin Dec 13, 2023

Choose a reason for hiding this comment

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

If you are referring to anonymous nodes, the name part might need to be 'subject_prefix' not $.subject_prefix. See
nvim-treesitter/nvim-treesitter#5774 (comment) --- I'm pretty sure the parser is up-to-date. Maybe it's fine on the treesitter side (because the test parse), but somehow nvim's treesitter parse does not recognize this rule (or node_type). There is clearly an error on the neovim's query parsing side.

Choose a reason for hiding this comment

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

Copy link

@wookayin wookayin Dec 13, 2023

Choose a reason for hiding this comment

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

If you look at the generated parser.c, neither ts_symbol_metadata nor ts_symbol_names, etc. contains sym_subject_prefix. I think the neovim's bundled libtree-sitter (I tested with 0.9.4 and 0.10-devel on macOS) is implemented in such a way that if that is not recognized as a valid node type, a TSQueryErrorNodeType error will be thrown. Maybe we have a different tree-sitter lib/version linked. Anyway, the generated parser's node definition doesn't look correct to me.

Choose a reason for hiding this comment

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

Copy link

@wookayin wookayin Dec 13, 2023

Choose a reason for hiding this comment

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

It's definitely fine. For example,
tree-sitter/tree-sitter-c@25371f9/grammar.js#L161-L163

      alias($.preproc_unary_expression, $.unary_expression),
      alias($.preproc_binary_expression, $.binary_expression),
      alias($.preproc_parenthesized_expression, $.parenthesized_expression),

I meant, it's fine to use $.unary_expression but the difference here is that there exist rules for all these three node types. https://github.com/tree-sitter/tree-sitter-c/blob/25371f9448b97c55b853a6ee8bb0bfb1bca6da9f/grammar.js#L929. $.subject_prefix (gitcommit) does not.

And I didn't re-generate parser.c, the one that was built in commit 7e3ad5f was used so I'm not sure if this is x86 vs arm64 issue.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for investigations @wookayin but I'm retty sure that's it's not a grammar issue.
@lucario387 I'm building on x86 but I've switched to node 20 and node-gyp 10, maybe I should retry using node 18 and node-gyp 9 ?
Thanks for your help :)

Choose a reason for hiding this comment

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

I don't know, I got the same generated parser.c as you, running on x86 :)
@amaanq will need to jump in to help here I feel

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've tried to build using docker, node 18, node 20, node-gyp 9, node-gyp 10, python 3.11, python 3.12 but I still get the same parser.c...
I've created an issue here if you prefer : #51

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