Change gdscript grammar provider

pastra98 commented 3 years ago

The repository currently responsible for providing syntax highlighting for gdscript files is broken, and doesn't seem to be maintained. I suggest switching to another repo that is updated more frequently.

Preliminary Steps

Please confirm you have...

[x] considered implementing an override,
[x] reviewed How Linguist Works,
[x] reviewed the Troubleshooting docs,
[x] verified an issue has not already been logged for your issue (linguist issues).

Problem Description

@lildude has already noticed this problem and submitted an issue here, which whas closed after the maintainer @beefsack added the yaml file back to the repository with this commit. However, since then several issues regarding the grammar have been opened, the biggest one being it's inability to handle typed function returns. This bug messes up the syntax highlighting of pretty much every gdscript repo that uses this feature, see this example.

There are other issues with the syntax highlighting as well, and even though linguists guidelines ask to discuss these in the actual repo, it doesn't seem like the maintainer is responding to new issues.

With the number of gdscript programs on github growing every day, I think it as shame that almost every file has completely messed up syntax highlighting. There are numerous repositories that implement gdscript syntax highlighting, most of which are updated more frequently, see these for example:

the vs code tools are a part of the godotengine account and are updated frequently
the intellij plugin seems to be updated frequently
as well as the atom plugin
or emacs plugin

I would have submitted a PR already, but I don't know where to find the appropriate file to fix this in this repository.

URL of the affected repository:

https://github.com/beefsack/GDScript-sublime

aaronfranke commented 3 years ago

When creating a solution to this issue, it should be designed to work with the GDScript rewrite that will be present in Godot 4.0. For more information, see this PR and build Godot's master branch.

lildude commented 3 years ago

@pastra98 please feel free to submit a PR to change the grammar source. Details are in the CONTRIBUTING.md file.

pastra98 commented 3 years ago

@lildude Okay, I'll try to set up linguist on my linux machine (I think that will be easier than windows?) after the weekend, but then it should simply be a case of running script/add-grammar --replace gdscript https://github.com/godotengine/godot-vscode-plugin , right? The grammar file should be text-mate compatible, I think.

Sorry for asking so many questions, just wanted to let you know I'll get to it soon🙂. I'm still a bloody noob when it comes to working on projects other than my own - or anything of decent complexity for that matter haha.

@aaronfranke But that shouldn't be an issue because it should be picked up from the upstream of the vscode plugin, right? And I assume they'll update it frequently.

Also, I hope this didn't come off as shitting on the work that @beefsack did, I appreciate everyone that does something for foss software

lildude commented 3 years ago

Okay, I'll try to set up linguist on my linux machine (I think that will be easier than windows?) after the weekend, but then it should simply be a case of running script/add-grammar --replace gdscript https://github.com/godotengine/godot-vscode-plugin, right?

Yup.

The grammar file should be text-mate compatible, I think.

Yup.

Sorry for asking so many questions, just wanted to let you know I'll get to it soon🙂. I'm still a bloody noob when it comes to working on projects other than my own - or anything of decent complexity for that matter haha.

NP. 👍

pastra98 commented 3 years ago

Thanks, @lildude .Okay, after running the command, I got:

paul@paul-pc:~/Documents/other_repos/linguist$ sudo script/add-grammar --replace gdscript https://github.com/godotengine/godot-vscode-plugin
Checking docker is installed and running
$ docker ps
Deregistering: vendor/grammars/gdscript
$ git submodule deinit vendor/grammars/gdscript
$ git rm -rf vendor/grammars/gdscript
$ script/grammar-compiler update -f
Registering new submodule: vendor/grammars/godot-vscode-plugin
$ git submodule add -f https://github.com/godotengine/godot-vscode-plugin vendor/grammars/godot-vscode-plugin
$ script/grammar-compiler add vendor/grammars/godot-vscode-plugin
  > latest: Pulling from linguist/grammar-compiler
  > Digest: sha256:01205d97572b3526f512ea8eacd06a0e02e033cf9364152d1742f314043e7964
  > Status: Image is up to date for linguist/grammar-compiler:latest
  > source 'vendor/grammars/godot-vscode-plugin' contains no grammar files
Command failed. Aborting.

After which I ran the tests, which got me:

paul@paul-pc:~/Documents/other_repos/linguist$ bundle exec rake test
cp lib/linguist/samples.json tmp/x86_64-linux/stage/lib/linguist/samples.json
install -c tmp/x86_64-linux/linguist/2.7.0/linguist.so lib/linguist/linguist.so
cp tmp/x86_64-linux/linguist/2.7.0/linguist.so tmp/x86_64-linux/stage/lib/linguist/linguist.so
/home/paul/Documents/other_repos/linguist/Rakefile:38: warning: calling URI.open via Kernel#open is deprecated, call URI.open directly or use URI#open
/home/paul/Documents/other_repos/linguist/vendor/gems/ruby/2.7.0/gems/charlock_holmes-0.7.7/lib/charlock_holmes/encoding_detector.rb:66: warning: instance variable encoding_list not initialized
Run options: --seed 46157

# Running:

.......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F........F.......................................................................................................................................................................FF.............

Finished in 23.286604s, 58.7033 runs/s, 1197.6414 assertions/s.

  1) Failure:
TestPedantic#test_submodules_are_sorted [/home/paul/Documents/other_repos/linguist/test/test_pedantic.rb:50]:
Expected false to be truthy.

  2) Failure:
TestLanguage#test_all_languages_have_grammars [/home/paul/Documents/other_repos/linguist/test/test_language.rb:367]:
The following languages' scopes are not listed in grammars.yml. Please add grammars for all new languages.
If no grammar exists for a language, mark the language with `tm_scope: none` in lib/linguist/languages.yml.
GDScript      source.gdscript
Q#            source.qsharp
SystemVerilog source.systemverilog

  3) Failure:
TestGrammars#test_readme_file_is_in_sync [/home/paul/Documents/other_repos/linguist/test/test_grammars.rb:42]:
Grammar list is out-of-date. Run `script/list-grammars`.
--- expected
+++ actual
@@ -131,7 +131,6 @@
 - **GAP:** [dhowden/gap-tmbundle](https://github.com/dhowden/gap-tmbundle)
 - **GCC Machine Description:** [textmate/lisp.tmbundle](https://github.com/textmate/lisp.tmbundle)
 - **GDB:** [quarnster/SublimeGDB](https://github.com/quarnster/SublimeGDB)
-- **GDScript:** [beefsack/GDScript-sublime](https://github.com/beefsack/GDScript-sublime)
 - **GEDCOM:** [fguitton/vscode-gedcom](https://github.com/fguitton/vscode-gedcom)
 - **GLSL:** [euler0/sublime-glsl](https://github.com/euler0/sublime-glsl)
 - **GN:** [devoncarew/language-gn](https://github.com/devoncarew/language-gn)
@@ -335,7 +334,6 @@
 - **Python:** [MagicStack/MagicPython](https://github.com/MagicStack/MagicPython)
 - **Python console:** [MagicStack/MagicPython](https://github.com/MagicStack/MagicPython)
 - **Python traceback:** [MagicStack/MagicPython](https://github.com/MagicStack/MagicPython)
-- **Q#:** [microsoft/qsharp-compiler](https://github.com/microsoft/qsharp-compiler)
 - **QML:** [skozlovf/Sublime-QML](https://github.com/skozlovf/Sublime-QML)
 - **QMake:** [textmate/cpp-qt.tmbundle](https://github.com/textmate/cpp-qt.tmbundle)
 - **Qt Script:** [atom/language-javascript](https://github.com/atom/language-javascript)
@@ -412,7 +410,6 @@
 - **SuperCollider:** [supercollider/language-supercollider](https://github.com/supercollider/language-supercollider)
 - **Svelte:** [umanghome/svelte-atom](https://github.com/umanghome/svelte-atom)
 - **Swift:** [textmate/swift.tmbundle](https://github.com/textmate/swift.tmbundle)
-- **SystemVerilog:** [bitbucket:Clams/sublimesystemverilog](https://bitbucket.org/Clams/sublimesystemverilog)
 - **TLA:** [agentultra/TLAGrammar](https://github.com/agentultra/TLAGrammar)
 - **TOML:** [textmate/toml.tmbundle](https://github.com/textmate/toml.tmbundle)
 - **TSQL:** [beau-witter/language-tsql](https://github.com/beau-witter/language-tsql)

  4) Failure:
TestGrammars#test_submodules_are_in_sync [/home/paul/Documents/other_repos/linguist/test/test_grammars.rb:36]:
The following submodules exist in the repository but aren't listed in grammars.yml.
Either add them to grammars.yml or remove them from the repository using `git rm`.
vendor/grammars/godot-vscode-plugin
vendor/grammars/qsharp-compiler

1367 runs, 27889 assertions, 4 failures, 0 errors, 0 skips
rake aborted!
Command failed with status (1)
/home/paul/Documents/other_repos/linguist/vendor/gems/ruby/2.7.0/gems/rake-13.0.1/exe/rake:27:in `<top (required)>'
/home/paul/Documents/other_repos/linguist/vendor/gems/ruby/2.7.0/bin/ruby_executable_hooks:24:in `eval'
/home/paul/Documents/other_repos/linguist/vendor/gems/ruby/2.7.0/bin/ruby_executable_hooks:24:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)

Is there something wrong with vscodes grammar files? How can I get this to work?

Alhadis commented 3 years ago

You ran script/add-grammar using sudo. You shouldn't have needed that — it can affect the permissions of files it creates or downloads, meaning only the superuser can modify them.

That being said, many of the errors you've posted relate to tasks that add-grammar was added to automate. You might want to try again with a fresh checkout, and NOT switch the grammar with superuser privileges.

pastra98 commented 3 years ago

@Alhadis

You ran script/add-grammar using sudo. You shouldn't have needed that — it can affect the permissions of files it creates or downloads, meaning only the superuser can modify them.

That being said, many of the errors you've posted relate to tasks that add-grammar was added to automate. You might want to try again with a fresh checkout, and NOT switch the grammar with superuser privileges.

Thanks for helping me out here. I have run the tests on a fresh checkout again, no failures. And ran add-grammar without superuser privileges after adding myself to a docker group. Got me the same result...

paul@paul-pc:~/Documents/other_repos/linguist$ script/add-grammar --replace gdscript https://github.com/godotengine/godot-vscode-plugin
Checking docker is installed and running
$ docker ps
Deregistering: vendor/grammars/gdscript
$ git submodule deinit vendor/grammars/gdscript
$ git rm -rf vendor/grammars/gdscript
$ script/grammar-compiler update -f
Registering new submodule: vendor/grammars/godot-vscode-plugin
$ git submodule add -f https://github.com/godotengine/godot-vscode-plugin vendor/grammars/godot-vscode-plugin
$ script/grammar-compiler add vendor/grammars/godot-vscode-plugin
  > latest: Pulling from linguist/grammar-compiler
  > Digest: sha256:01205d97572b3526f512ea8eacd06a0e02e033cf9364152d1742f314043e7964
  > Status: Image is up to date for linguist/grammar-compiler:latest
  > source 'vendor/grammars/godot-vscode-plugin' contains no grammar files
Command failed. Aborting.

I have no clue... Should the grammar file be named in some special way for this to work?

Alhadis commented 3 years ago

> source 'vendor/grammars/godot-vscode-plugin' contains no grammar files

Oops, I missed that the first time. That shouldn't have happened. 🤔

Should the grammar file be named in some special way for this to work?

Most of the VSCode grammar repos I've come across place the .tmLanguage file in the root directory, though it's often generated from some other format. I'm not a VSCode user, nor am I an expert on where the grammar-compiler looks for TextMate-compatible grammars in VSCode packages.

The only other thing I can think of is to double-check that vendor/grammars/godot-vscode-plugin was actually initialised (cloned), and isn't simply an empty directory.

pastra98 commented 3 years ago

Most of the VSCode grammar repos I've come across place the .tmLanguage file in the root directory, though it's often generated from some other format. I'm not a VSCode user, nor am I an expert on where the grammar-compiler looks for TextMate-compatible grammars in VSCode packages.

Hmm, I looked through some of the other repos in vendors/, and others do have the tm file in a deeper directory.

The only other thing I can think of is to double-check that vendor/grammars/godot-vscode-plugin was actually initialised (cloned), and isn't simply an empty directory.

Yeah, It's all there, including configurations/GDScript.tmLanguage.json.

Unfortunately I don't really know much about shell scripting (windows pleb here), but I looked through add-grammar and grammar-compiler... Tbh I don't understand much of whats going on there😞

I'll try with the atom repo later, though it looks very similar to the vscode one.

Alhadis commented 3 years ago

including configurations/GDScript.tmLanguage.json

Remove the .json part of the filename so its extension is just .tmLanguage, then try again.

pastra98 commented 3 years ago

Remove the .json part of the filename so its extension is just .tmLanguage, then try again.

funny... I forked the vscode repo, renamed, and ran it again... apparently the compiler buggs out when trying to read the comparison operators. Well, first off - here's what the script told me:

paul@paul-pc:~/Documents/other_repos/linguist$ script/add-grammar --replace gdscript https://github.com/pastra98/godot-vscode-plugin
Checking docker is installed and running
$ docker ps
Deregistering: vendor/grammars/gdscript
$ git submodule deinit vendor/grammars/gdscript
$ git rm -rf vendor/grammars/gdscript
$ script/grammar-compiler update -f
Registering new submodule: vendor/grammars/godot-vscode-plugin
$ git submodule add -f https://github.com/pastra98/godot-vscode-plugin vendor/grammars/godot-vscode-plugin
$ script/grammar-compiler add vendor/grammars/godot-vscode-plugin
  > latest: Pulling from linguist/grammar-compiler
  > Digest: sha256:01205d97572b3526f512ea8eacd06a0e02e033cf9364152d1742f314043e7964
  > Status: Image is up to date for linguist/grammar-compiler:latest
  > The new grammar repository `vendor/grammars/godot-vscode-plugin` (from https://github.com/pastra98/godot-vscode-plugin) contains 1 errors:
  >     - Grammar conversion failed. File `configurations/GDScript.tmLanguage` failed to parse: XML syntax error on line 93: expected element name after <
  > 
  > failed to compile the given grammar
Command failed. Aborting.

Here's line 93 from the renamed grammar:

      "match": "<=|>=|==|<|>|!=",

Not sure if this is bad design in the vscode repo, or if it is an issue with linguist... I don't wan't to give you guys too much of a headache haha, sorry about that. I tried to wrap my head around the compiler, but I'll have to give that some more time. Maybe the issue is quite obvious, but not for me.

Alhadis commented 3 years ago

failed to parse: XML syntax error on line 93: expected element name after

Ugh, sorry. I forgot .tmLanguage is actually an XML-based format; the JSON extension I was thinking of is .json‑tmLanguage (which isn't supported anyway, so, never mind…).

Now, here's where and what the compiler is searching for. .tmLanguage.json file should be moved to syntaxes/.tmlanguage.json. So were right in the first place, it wasn't in the right location. 😅

pastra98 commented 3 years ago

Holy macaroni, that worked! Thank you so much @Alhadis ! I made the directory, and it did not complain! But the tests were complaining about qsharp for some reason, lol:

paul@paul-pc:~/Documents/my_repos/linguist$ bundle exec rake testcp lib/linguist/samples.json tmp/x86_64-linux/stage/lib/linguist/samples.json
install -c tmp/x86_64-linux/linguist/2.7.0/linguist.so lib/linguist/linguist.so
cp tmp/x86_64-linux/linguist/2.7.0/linguist.so tmp/x86_64-linux/stage/lib/linguist/linguist.so
/home/paul/Documents/my_repos/linguist/Rakefile:38: warning: calling URI.open via Kernel#open is deprecated, call URI.open directly or use URI#open
/home/paul/Documents/my_repos/linguist/vendor/gems/ruby/2.7.0/gems/charlock_holmes-0.7.7/lib/charlock_holmes/encoding_detector.rb:66: warning: instance variable encoding_list not initialized
Run options: --seed 46342

# Running:

............................................F.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F....................................................

Finished in 22.644105s, 60.3689 runs/s, 1231.6230 assertions/s.

  1) Failure:
TestGrammars#test_submodules_are_in_sync [/home/paul/Documents/my_repos/linguist/test/test_grammars.rb:36]:
The following submodules exist in the repository but aren't listed in grammars.yml.
Either add them to grammars.yml or remove them from the repository using `git rm`.
vendor/grammars/qsharp-compiler

  2) Failure:
TestLanguage#test_all_languages_have_grammars [/home/paul/Documents/my_repos/linguist/test/test_language.rb:367]:
The following languages' scopes are not listed in grammars.yml. Please add grammars for all new languages.
If no grammar exists for a language, mark the language with `tm_scope: none` in lib/linguist/languages.yml.
Q#            source.qsharp
SystemVerilog source.systemverilog

1367 runs, 27889 assertions, 2 failures, 0 errors, 0 skips
rake aborted!
Command failed with status (1)
/home/paul/Documents/my_repos/linguist/vendor/gems/ruby/2.7.0/gems/rake-13.0.1/exe/rake:27:in `<top (required)>'
/home/paul/Documents/my_repos/linguist/vendor/gems/ruby/2.7.0/bin/ruby_executable_hooks:24:in `eval'
/home/paul/Documents/my_repos/linguist/vendor/gems/ruby/2.7.0/bin/ruby_executable_hooks:24:in `<main>'
Tasks: TOP => test
(See full trace by running task with --trace)

Well, I ran the tests before changing the grammar- No issues. But before that I tried it with the atom plugin, which also worked out, and gave me the same issue with qsharp. Pretty sure this is an unrelated bug.

Anyhow, I'm pretty new to this whole github collaboration thing. Should I make a PR in the vscode repo to add this directory? Not sure if they will understand... But it is working in my fork now (: Thanks again for helping me out so much!

Alhadis commented 3 years ago

Should I make a PR in the vscode repo to add this directory? Not sure if they will understand...

Yes, but there may be config files that need updating if the grammar file is moved. I'm sure they'll let you know. 😉

pastra98 commented 3 years ago

Okay, I looked through the atom repository and tested it out with the atom editor. And it works quite well with the current version of gdscript. I figure this will be easier than making the changes to the vscode repo. I looked at the files that would need to change there, and because the rest of the files in /configurations don't have anything to do with grammar itself- it would'nt make sense to rename the entire folder to /syntaxes or even make a folder just for the grammar file. The atom repository doesn't need changes like that.

I think I learned some things about textmate style grammar files, so I feel more confident to try to help fix some issues, should they creep up in the new repository. But right now the syntax highlighting there is up to date, and suitable for linguist.

Please tell me if I forgot something/should fix something. I ran the tests after my changes, and everything seems to be fine. Thanks again @Alhadis for mentoring me through the process, I hope this can be merged soon (:

pastra98 commented 3 years ago

Okay, thanks to @Razoric480 and @Calinou the vscode repository was updated, and the tmlanguage file is now in the /syntaxes folder. I deleted the branch that I used to change to the atom repository in the previous pull request. I think everything should be working now...

github-linguist / linguist