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

Better handling of generated files #29

Closed
resolritter opened this issue Apr 22, 2023 · 1 comment · Fixed by #33
Closed

Better handling of generated files #29

resolritter opened this issue Apr 22, 2023 · 1 comment · Fixed by #33
Assignees

Comments

@resolritter
Copy link
Collaborator

resolritter commented Apr 22, 2023

tree-sitter generate updates the parser's files after grammar changes. Tree-sitter grammars usually adopt one of the following strategies for dealing with those files:

1 - Commit the changes to the main branch through PRs, as done in e.g. tree-sitter-typescript; see https://github.com/tree-sitter/tree-sitter-typescript/pull/241/files for example
2 - Commit the changes to a release branch, as done in e.g. tree-sitter-swift; see alex-pinkus/tree-sitter-swift#149 (comment) and https://github.com/alex-pinkus/tree-sitter-swift/tree/with-generated-files for example

I infer that this repository wants to abide by strategy 1. If so, a CI check should be added to ensure that the parser's files are up-to-date when a PR is submitted. The lack of such check is detrimental to the grammar because, otherwise, reviewers have to remember to ask the PR submitters to update and commit the files before merge; as an aside, the reminder did come up when I submitted #26 and as a result the parser's files in main are currently outdated. That could have been prevented with a CI check along those lines:

make generate
git diff-index --quiet HEAD || \
  { >&2 echo 'Please run `make generate` and commit the files'; exit 1; }

Please let me know if you want to implement that check on CI or do it some other way.

@resolritter
Copy link
Collaborator Author

For what it's worth, I think strategy 2 is cleaner wrt. the commit history. In #33 I went with strategy 1 because it's more in line with what's already done in this repository.

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 a pull request may close this issue.

1 participant