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

Replace SourcePawn grammar #6635

Merged
merged 10 commits into from
Mar 8, 2024
Merged

Replace SourcePawn grammar #6635

merged 10 commits into from
Mar 8, 2024

Conversation

Sarrus1
Copy link
Contributor

@Sarrus1 Sarrus1 commented Dec 17, 2023

Replace SourcePawn grammar

Description

Dreae's grammar is no longer maintained and is missing a few Sourcepawn core features. This PR replaces it with mine, which now properly handles the old and new syntax, most importantly type highlighting for variable declarations.

Checklist:

@Sarrus1 Sarrus1 requested a review from a team as a code owner December 17, 2023 16:20
@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Dec 17, 2023

A couple more things:

1

I would also like to point out my fully functional tree-sitter grammar for Sourcepawn here.
It has highlighting queries, as well as tags queries for Github navigation support.
From my understanding of the discussion here, it is not possible to contribute tree-sitter grammars to linguist. Is this still the case? Is there a way to get the tree-sitter grammar into Github by any chance?

2

The textmate grammar supports JSDoc injections, is there something I should do to add support to that in Linguist as well?
https://github.com/Sarrus1/sourcepawn-vscode/blob/main/editors/code/syntaxes/sourcepawn-jsdoc.json

3

This repo can be deleted now: https://github.com/github-linguist/sublime-sourcepawn

Thanks for taking the time to consider this PR!

@lildude
Copy link
Member

lildude commented Dec 18, 2023

Is this still the case?

Yes

Is there a way to get the tree-sitter grammar into Github by any chance?

No

The textmate grammar supports JSDoc injections, is there something I should do to add support to that in Linguist as well?

I'm not sure what there would be to add. Linguist only acts as a collection point which bundles them up for the syntax highlighting engine which is what reads the grammars. No processing of the files takes place other than some basic PCRE regex checking and then conversion to JSON if not already in JSON format. We expect the grammar file to be in its final and complete state as no other processing takes place.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the script/add-grammar script to replace the grammar as documented in the CONTRIBUTING.md file, and don't manually modify any of the files updated by the script.

@lildude lildude changed the title fix: replace SourcePawn grammar Replace SourcePawn grammar Dec 18, 2023
Copy link
Contributor Author

@Sarrus1 Sarrus1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 there should be more changes than this. Your repo has several grammar scope names so the grammars.yml file should have been updated to include these. The cached license file should also have changed as the commits are different. Are you sure you've added all the changed files?

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Dec 19, 2023

🤔 there should be more changes than this. Your repo has several grammar scope names so the grammars.yml file should have been updated to include these. The cached license file should also have changed as the commits are different. Are you sure you've added all the changed files?

I reran the script just to be sure, here are the commands I ran:

> script/add-grammar --replace "sourcepawn-vscode" https://github.com/Sarrus1/sourcepawn-vscode
Checking Docker is installed and running
Deregistering submodule: vendor/grammars/sourcepawn-vscode
Cleared directory 'vendor/grammars/sourcepawn-vscode'
Submodule 'vendor/grammars/sourcepawn-vscode' (https://github.com/Dreae/sourcepawn-vscode) unregistered for path 'vendor/grammars/sourcepawn-vscode'
rm 'vendor/grammars/sourcepawn-vscode'
Registering new submodule: https://github.com/Sarrus1/sourcepawn-vscode.git
Reactivating local git directory for submodule 'vendor/grammars/sourcepawn-vscode'
latest: Pulling from linguist/grammar-compiler
Digest: sha256:fcaf08a6e27d3d88212d9ca8aceee30d4089499a77bb05307871febd8073790f
Status: Image is up to date for linguist/grammar-compiler:latest
docker.io/linguist/grammar-compiler:latest

What's Next?
  1. Sign in to your Docker account → docker login
  2. View a summary of image vulnerabilities and recommendations → docker scout quickview linguist/grammar-compiler:latest
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
OK! added grammar source 'vendor/grammars/sourcepawn-vscode'
        new scope: source.sourcepawn
Caching grammar license
Caching dependency records for linguist
  git_submodule
    Using AL (96c407663984341b21e574a58ef6ca6f89cc3fc0)
    Using Alloy.tmbundle (0a80fc74e921325da23264889771a88a02877414)
    Using Assembly-Syntax-Definition (519b7b3c5f8428832dde72a90041d0707f831e5c)
    .... # cropping some of it
  * 451 git_submodule dependencies
Updating grammar documentation in vendor/README.md

> git status
On branch master
Your branch is behind 'origin/master' by 1 commit, and can be fast-forwarded.
  (use "git pull" to update your local branch)

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   .gitmodules

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   .gitmodules
	modified:   Brewfile
	modified:   vendor/README.md

> git add --all
> git commit -m "fix: replace SourcePawn grammar"
> git push -f

The grammar is located in this folder which has a parent directory with an MIT License. Maybe the location is a problem?

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Dec 26, 2023

Hello @lildude I think I finally managed to make the script work properly. All the changes should be in order now 👍

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 7, 2024

Hello, bumping this. Is there anything else I can do to improve this PR?

(I will start by rebasing)

@lildude
Copy link
Member

lildude commented Feb 7, 2024

There’s no rush and not need to rebase as this PR won’t be merged until close to the next release anyway and that will include a merge update if needed.

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this earlier, but we can not accept this change.

Whilst the grammar files are licensed under MIT, the over-all repo is licensed by GPL-3 which means we'll potentially be shipping code that is covered by this license which we can't do.

I'm also not sure GPL3 allows sub-licensing.

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

Should I move my grammar to a new repo to solve the issue?

@lildude
Copy link
Member

lildude commented Feb 20, 2024

That would probably be best short of relicensing the current repo, but that's entirely up to you. We can't accept the current repo as it stands with the GPL-3 license.

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

Changing the License to MIT is fine with me 👍
I have changed it on the main branch and feat/salsa which is currently the default branch until it is merged.
Should I rerun something to account for that?

@lildude
Copy link
Member

lildude commented Feb 20, 2024

Should I rerun something to account for that?

Yes. Re-running script/add-grammar --replace... like you did when switching the grammars should pull in the updated license file.

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

All done 👍

grammars.yml Outdated Show resolved Hide resolved
@lildude
Copy link
Member

lildude commented Feb 20, 2024

The changes to vendor/licenses/git_submodule/sourcepawn-vscode.dep.yml are also not consistent with what I'd have expected the various scripts to have done. This looks like a manual change.

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

This looks like a manual change.

I did the manual change in a previous commit and just reran the script thinking it would overwrite it.

I have now reran the script again and made sure that it edited the files correctly.

vendor/README.md Outdated Show resolved Hide resolved
vendor/README.md Outdated Show resolved Hide resolved
@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

I updated the commit as I had to force push a change on the main branch of the repo.

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 20, 2024

I'm not sure what I should do to get the script to recognize that the repo has an MIT license instead of "others".

@Sarrus1
Copy link
Contributor Author

Sarrus1 commented Feb 21, 2024

I think everything should be in order now! The commit version is correct 👍

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

Note: this PR will not be merged until close to when the next release is made. See here for more details.

@lildude lildude added this pull request to the merge queue Mar 8, 2024
Merged via the queue into github-linguist:master with commit a42eb2d Mar 8, 2024
5 checks passed
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants