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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v3
with:
node-version: '18.x'
node-version: '20.10.x'
registry-url: 'https://registry.npmjs.org'
- name: Install dependencies
run: npm ci
- name: Run tree-sitter tests
run: npm test
- name: Ensure generated parser files are up-to-date
# On Windows, tree-sitter generate results in a diff, not sure why
if: runner.os != 'Windows'
# On Windows and Macos, tree-sitter generate results in a diff, not sure why
if: runner.os != 'Windows' && runner.os != 'macOS'
run: |
git status
test -z "$(git status --porcelain)"
57 changes: 55 additions & 2 deletions corpus/subject.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ This should not exists
--------------------------------------------------------------------------------

(source
(subject)
(ERROR))
(subject)
(ERROR))

================================================================================
Subject with type and scope
Expand Down Expand Up @@ -157,3 +157,56 @@ feat(issue42): allow numbers in scope

(source
(subject))

================================================================================
Parse fixup!
================================================================================
fixup! allow fixup

--------------------------------------------------------------------------------

(source
(subject
(subject_prefix)))

================================================================================
Parse amend! with conventional commit
================================================================================
amend! feat(test): allow fixup

--------------------------------------------------------------------------------

(source
(subject
(subject_prefix)
(prefix
(type)
(scope))))

================================================================================
Parse amend! with conventional commit and overflow
================================================================================
amend! feat(test): allow fixup with an overflow on the subject

--------------------------------------------------------------------------------

(source
(subject
(subject_prefix)
(prefix
(type)
(scope))
(overflow)))

================================================================================
Parse fixup! with conventional commit and no scope
================================================================================
amend! feat: allow fixup

--------------------------------------------------------------------------------

(source
(subject
(subject_prefix)
(prefix
(type))))
3 changes: 3 additions & 0 deletions grammar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

Copy link

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,
https://github.com/tree-sitter/tree-sitter-c/blob/25371f9448b97c55b853a6ee8bb0bfb1bca6da9f/grammar.js#L161-L163

Copy link

@wookayin wookayin Dec 13, 2023 •
edited
Loading

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.

Copy link

Choose a reason for hiding this comment

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

Sounds like a tree-sitter-grammars/tree-sitter-markdown@110b89d

Copy link

@wookayin wookayin Dec 13, 2023 •
edited
Loading

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 :)

Copy link

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

),
choice(
seq(NOT_A_COMMENT, SUBJECT),
seq($.prefix, $._conventional_subject)
Expand Down
27 changes: 7 additions & 20 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions queries/highlights.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@
(breaking_change (value) @text)

(scissor) @comment
(subject_prefix) @keyword

(ERROR) @error
35 changes: 35 additions & 0 deletions src/grammar.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,41 @@
"subject": {
"type": "SEQ",
"members": [
{
"type": "CHOICE",
"members": [
{
"type": "SEQ",
"members": [
{
"type": "ALIAS",
"content": {
"type": "CHOICE",
"members": [
{
"type": "STRING",
"value": "fixup!"
},
{
"type": "STRING",
"value": "amend!"
}
]
},
"named": true,
"value": "subject_prefix"
},
{
"type": "PATTERN",
"value": "[\\f\\v ]+"
}
]
},
{
"type": "BLANK"
}
]
},
{
"type": "CHOICE",
"members": [
Expand Down
8 changes: 8 additions & 0 deletions src/node-types.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,10 @@
{
"type": "prefix",
"named": true
},
{
"type": "subject_prefix",
"named": true
}
]
}
Expand Down Expand Up @@ -1060,6 +1064,10 @@
"type": "squash",
"named": false
},
{
"type": "subject_prefix",
"named": true
},
{
"type": "supprimé :",
"named": false
Expand Down
Loading