jrmoulton / tree-sitter-slint

A Tree Sitter parser for slint
MIT License
3 stars 2 forks source link

chore: update slint to v1 grammar #8

Closed Decodetalkers closed 1 year ago

Decodetalkers commented 1 year ago

cc @jrmoulton

jrmoulton commented 1 year ago

Thanks for the PR!

Currently the status of tree-sitter-slint is in a bad spot.

The slint main repository had an old version of tree-sitter-slint that was there because of a partial attempt to migrate this repository to the main slint repository.

But that migration was never really completed so a few months ago I did a fairly major rewrite of the grammar here in this repository.

But then the slint folks updated their old version to the 1.0 syntax without merging my rewrite. Ugh.

I think I would like to keep this version of the grammar as it is much cleaner. But we need to check that with this PR all of the tests that are currently in the slint repository pass

This is a link to the PR where they updated to the 1.0 syntax. https://github.com/slint-ui/slint/pull/2144

Link to a the python file that generates all of the relevant tests. https://github.com/slint-ui/slint/blob/5f239124dee9dd0962c2832e1590449265f5cfb7/editors/tree-sitter-slint/test-to-corpus.py

If you can include the tests (I would like to have the tests committed to this repository) and the passing status I will go ahead and merge this PR. Otherwise I'll get around to verifying all of the tests in the next week or so.

Decodetalkers commented 1 year ago

you mean add a ci, I will take a try

I have already update the test, and I found that the test under slint is not quite enough than here, so I not copy their test, because the test here is enough I think

and I don't know if you like the format of prettier >_<

Decodetalkers commented 1 year ago

Then I want to solve some bugs.. yes.. there are still pretty much problems of this scheme.. so when will you have time continue? I will come to help

Decodetalkers commented 1 year ago

https://github.com/DerekStride/tree-sitter-sql/blob/main/.github/workflows/gh-pages.yml

There ci is cool, they add src to gitignore, and push the production to gh-pages.I think we should do as the same,but before delete the source under src , we should create such production first, and change the branch on nvim-treesitter

hunger commented 1 year ago

The checked in tests in the slint repo for treesitter are basically a smoke test.

The interesting part is done by taking all the existing test data, turn that into a tresitter test run all of them in update mode, and then grep for Errors in the output. The hope is to catch any blanks in the parser that way, while pretty much ignoring any change in the output of the parser.

The cool thing here is that this is run each time somebody commits to the slint repository and will fail the CI if treesitter introduces Error nodes. So it blocks new language constructs going into the slint repository without also updating the treesitter grammar. I am totally open to replace any of the code I wrote with better implementations, but I would really like to keep this last property of failing CI when the treesitter grammar is not up to date.

hunger commented 1 year ago

Sorry, I was not aware you had a rewrite in this repo, so I did my slint 1.0 update in the slint repository. I would totally have gone for your rewrite if Inhad been aware.

It's my first attempt at treesitter, so it probably is far from ideal.

What is preventing you to merge into the main slint repository? If you do not want to merge, then I can also remove the treesitter we have in the main repository and point to this repository. I would personally prefer having all of slint in one repo (and treesitter isnpart of the complete package in my mind) though.

Sorry for bringing this up here, but you mentioned the mess earlier in this PR, so I thought I amend to that part here.

jrmoulton commented 1 year ago

Sorry, I was not aware you had a rewrite in this repo, so I did my slint 1.0 update in the slint repository. I would totally have gone for your rewrite if Inhad been aware.

It's my first attempt at treesitter, so it probably is far from ideal.

What is preventing you to merge into the main slint repository? If you do not want to merge, then I can also remove the treesitter we have in the main repository and point to this repository. I would personally prefer having all of slint in one repo (and treesitter isnpart of the complete package in my mind) though.

Sorry for bringing this up here, but you mentioned the mess earlier in this PR, so I thought I amend to that part here.

@hunger No worries. It's my own fault for not getting them merged and finalized. The only reason I still made changes here was because the discussion around how to handle which repo nvim treesitter should download because of the size of the main repo. But I should've gotten the merge finalized. Now that we are for sure going to merge this into Slint and point nvim treesitter to the main Slint repo I'll delete this repository as soon as we get this merged with all of the tests passing and all of the queries finalized.

Decodetalkers commented 1 year ago

The checked in tests in the slint repo for treesitter are basically a smoke test.

The interesting part is done by taking all the existing test data, turn that into a tresitter test run all of them in update mode, and then grep for Errors in the output. The hope is to catch any blanks in the parser that way, while pretty much ignoring any change in the output of the parser.

The cool thing here is that this is run each time somebody commits to the slint repository and will fail the CI if treesitter introduces Error nodes. So it blocks new language constructs going into the slint repository without also updating the treesitter grammar. I am totally open to replace any of the code I wrote with better implementations, but I would really like to keep this last property of failing CI when the treesitter grammar is not up to date.

I think slint can take tree-sitter-sql as example, it placed target to another branch, I do the same thing in my tree-sitter-groovy repo, maybe slint can update ci to like this

Decodetalkers commented 1 year ago

Sorry, I was not aware you had a rewrite in this repo, so I did my slint 1.0 update in the slint repository. I would totally have gone for your rewrite if Inhad been aware.

It's my first attempt at treesitter, so it probably is far from ideal.

What is preventing you to merge into the main slint repository? If you do not want to merge, then I can also remove the treesitter we have in the main repository and point to this repository. I would personally prefer having all of slint in one repo (and treesitter isnpart of the complete package in my mind) though.

Sorry for bringing this up here, but you mentioned the mess earlier in this PR, so I thought I amend to that part here.

https://github.com/Decodetalkers/tree-sitter-groovy/blob/master/.github/workflows/publishsource.yml

This is copy from tree-sitter-sql, the ci is good because when target is really changed, the branch will make a commit.. hope will be the help of you

hunger commented 1 year ago

Oh, I can make the Slint CI do a lot of things:-)

Right now slint has one big mono-repo with everything in it. This made it very convenient for us to work with the codebase, so we would like to keep that. The current tree-sitter in the mono repo does build from Mason, so it can be done technically.

The biggest downside I can see is that all the treesitter users end up downloading a bit more data than strictly necessary.

Decodetalkers commented 1 year ago

ping.. can it be merged , or what is the status of tree-sitter-slint on slint-ui? @hunger @jrmoulton

jrmoulton commented 1 year ago

I haven't had much time to work on it but I'll go ahead and merge this. Sorry I've taken so long to get to this