fwcd / tree-sitter-kotlin

Kotlin grammar for Tree-sitter
https://fwcd.github.io/tree-sitter-kotlin
MIT License
124 stars 53 forks source link

Add notes on contributing, especially w.r.t the parser verification check #92

Open fwcd opened 1 year ago

fwcd commented 1 year ago

New PRs regularly fail the CI check that verifies that the generated parser matches the committed parser.c, presumably because their development machine didn't generate a byte-for-byte-exact copy of the parser as the GitHub Actions runner.

Perhaps we could add some notes to clarify the motivation behind this check and how to successfully generate a conforming parser.c. Something along the lines of:

ketkarameya commented 1 year ago

Do we really need parser.c in the repo. The tree-sitter-swift or tree-sitter-java don't have it in the repo either.

https://github.com/alex-pinkus/tree-sitter-swift#frequently-asked-questions https://github.com/tree-sitter/tree-sitter-java

fwcd commented 1 year ago

tree-sitter-java does in fact check it in (see here), but I do like the approach of tree-sitter-swift, that is, omitting the parser and publishing it as a CI artifact. That would probably also help with repo growth, which may become problematic due to the frequent changes in parser.c.

The only reason I am a bit hesitant is that it seems to deviate from the dominant convention of including the parser, even if it's comparatively large. See e.g.

fwcd commented 1 month ago

Might be worth keeping an eye on

which propose to remove the generated files from the repos.

clason commented 1 month ago

...but also include it as a release artefact for semantic versioned(!) releases. (This parser takes a comparatively long time to generate, so just dropping it would not be preferred albeit workable. A separate "release branch", though, is highly discouraged -- some parsers chose to do that in the early days, but it's more headache for everyone than it's worth.)

In either case, please keep committing the generated json, as these are enough to generate the parser without node. (Not that your parser requires npm, as far as I can tell, but it's the principle.)

fwcd commented 1 month ago

Yes, to be clear, I don't propose to remove anything for now, just to monitor what upstream does. When the official recommendation changes, we can revisit this.

clason commented 1 month ago

Sure; I was just explaining current upstream policy ;)