search

tree-sitter / tree-sitter-typescript

TypeScript grammar for tree-sitter
MIT License
341 stars 104 forks source link

Add a CI step that checks whether the generated files are up-to-date #194

Closed hendrikvanantwerpen closed 2 years ago

hendrikvanantwerpen commented 2 years ago

This PR adds a step to the build workflow that builds the grammar and checks if the generated files change w.r.t. to the files in HEAD.

The goal is to prevent problems when the grammar is updated but the generated files are forgotten, which causes invalid tests results as these are ran using the generated files. By running npm run build in the workflow and checking that the generated files in HEAD are the same as the ones produced by the CI build, we prevent inconsistencies between grammar and generated files.

I have tested this locally using act, but I should note that I do not have a lot of experience with workflows.

This should prevent issues like #193 in the future.

hendrikvanantwerpen commented 2 years ago

I see that there is a plan to eliminate generated files from the grammar repositories entirely, which is of course an even better approach. But I think this is still valuable in the meantime.

mjambon commented 2 years ago

Sounds like a good idea! Will this complain if for example I just edit the readme and make a pull request? If so, maybe I'd check for changes in common/define-grammar.js.

hendrikvanantwerpen commented 2 years ago

Good question, @mjambon... If you don't make a grammar change, but the grammar and the generated files are already in an inconsistent state, then it will also complain, yes. That is currently the case for this PR: I've only added the consistency check, but because the repo is currently inconsistent (see #193), the checks for this PR are failing.

I am not sure if it is useful to check what was actually changed, though? The idea is that a PR which is not consistent would never be merged in the first place. So, if main is consistent, and there are no grammar changes, this test should never fail for that PR. So once we introduce the check on a consistent commit, main should remain consistent because inconsistencies cannot be merged.

mjambon commented 2 years ago

Oh, great. Sorry, I misunderstood the code.

Now we will also get errors if the committer didn't touch the grammar but tree-sitter was upgraded in CI. It's possible because the version constraint in package.json is "^0.20.0", which allows 0.20.1, 0.20.2 etc, so the latest 0.20.x version will be picked. In this case, the author will have to upgrade their tree-sitter locally and regenerate the files. That shouldn't be a big problem. In the worst case of someone who's not set up for development and just updates the docs, the reviewer of the PR can help.