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 Common Lisp grammar source #6728

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

qingpeng9802
Copy link
Contributor

Description

Things may be a bit more complicated than usual. This PR would like to replace the old grammar of Common Lisp with my grammar.

Common Lisp and other lisp dialects are currently using source.lisp as the scope. My grammar is strictly follow Common Lisp standard so it is not a good idea to use my grammar on other lisp dialects. Thus, the new grammar (source.commonlisp) is added for Common Lisp only, but the old grammar (source.lisp) is kept for other lisp dialects.

The grammar is originally from my repo vscode-common-lisp.

Checklist:

Signed-off-by: Qingpeng Li <43924785+qingpeng9802@users.noreply.github.com>
Signed-off-by: Qingpeng Li <43924785+qingpeng9802@users.noreply.github.com>
@qingpeng9802 qingpeng9802 requested a review from a team as a code owner February 29, 2024 16:10
@Lacters

This comment was marked as spam.

@Lacters

This comment was marked as spam.

Signed-off-by: Qingpeng Li <43924785+qingpeng9802@users.noreply.github.com>
@lildude
Copy link
Member

lildude commented Mar 5, 2024

The grammar is originally from my repo vscode-common-lisp.

Why not use that repo directly? From a quick look it looks like that repo has the grammars in a format we can use in a location we expect to find grammar files… we'll find the *.tmLanguage.json files. I note that the commonlisp grammar hasn't been touched in a while so it's likely your update won't be picked up be linguist too.

FYI, we look for these extensions:

var preferredGrammars = map[string]int{
".tmlanguage": 0,
".cson": 1,
".json": 1,
".plist": 2,
".yaml-tmlanguage": 3,
}

… in these locations:

switch strings.ToLower(ext) {
case ".plist":
return strings.HasSuffix(dir, "/Syntaxes")
case ".tmlanguage", ".yaml-tmlanguage":
return true
case ".cson", ".json":
return strings.HasSuffix(dir, "/grammars") || strings.HasSuffix(dir, "/syntaxes") || GrammarsInNonStdPath[filepath.Base(dir)]
default:
return false
}

@qingpeng9802
Copy link
Contributor Author

Why not use that repo directly?

This grammar aims to collaborate with the semantic token functionality of vscode. If two user groups use the same repo, potential GitHub users may request changes that are not suitable for upstreaming to the vscode repo. The demands of different user groups may break the expectations of the original functionality.

grammar hasn't been touched in a while

This grammar works fine for the current users from vscode. We respect and value the smoothness of user experience. Considering the lack of language feature support for lisp.tmbundle, we believe that changing it to a market-proof grammar can bring GitHub users more user experience value.

@lildude
Copy link
Member

lildude commented Mar 5, 2024

Cool. I was suggesting using the same repo for both to make things easier for you, but it's fine if you prefer to keep them separate and duplicate things as 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.

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 63dad32 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.

None yet

3 participants