Skip to content

Conversation

@Chocapikk
Copy link

@Chocapikk Chocapikk commented Feb 8, 2026

Description

Replaces the archived language-shellscript (Atom) grammar with better-shell-syntax by Jeff Hykin, which is actively maintained and used by VS Code.

The old grammar provided two scopes: source.shell and text.shell-session. Since better-shell-syntax only covers source.shell, the text.shell-session grammar has been extracted into a standalone repo (shell-session-syntax) and added as a separate submodule.

The grammar source uses an autogenerated/ directory for its tmLanguage files, which required adding "autogenerated" to GrammarsInNonStdPath in data.go so the grammar compiler can locate them.

The better-shell-syntax submodule currently points to my fork which includes a fix for variable-length lookbehind assertions (converted to fixed-length PCRE-compatible form). The upstream PR for this fix is pending: jeff-hykin/better-shell-syntax#128. Once merged, the submodule URL can be updated to point to the upstream repo.

Ref: #5831

Checklist:

@Chocapikk Chocapikk requested a review from a team as a code owner February 8, 2026 23:25
@Chocapikk Chocapikk force-pushed the replace-shell-grammar-with-better-shell-syntax branch 3 times, most recently from 9efef42 to 41a5e19 Compare February 9, 2026 00:32
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.

linguist/CONTRIBUTING.md

Lines 98 to 99 in e51c227

> [!IMPORTANT]
> Pull requests will not be reviewed if the template is not used or not filled in.

This PR is also invalid as you/AI did not followed the instructions in the CONTRIBUTING.md file for replacing a grammar.

@Chocapikk
Copy link
Author

I did use script/add-grammar --replace. Here's what happens step by step:

Step 1 script/add-grammar --replace language-shellscript https://github.com/jeff-hykin/better-shell-syntax fails with:

source 'vendor/grammars/better-shell-syntax' contains no grammar files

The grammar compiler doesn't recognize the autogenerated/ subdirectory. That's the reason for the data.go change adding "autogenerated": true to GrammarsInNonStdPath.

Step 2 After that one line fix, rebuilding the compiler (REBUILD=1) and rerunning the script:

1 warnings found when compiling new grammar:
- Unknown keys in grammar: `source.shell` contains invalid keys (`information_for_contributors`, `version`)
These warnings are not fatal [...]
OK! added grammar source 'vendor/grammars/better-shell-syntax'
    new scope: source.shell

It compiles and passes. The warnings are just VS Code metadata keys (information_for_contributors, version), not errors.

So this wasn't "not following instructions". The script cannot handle this grammar without the data.go fix, everything else in the diff is exactly what the script produces.

I can close this PR if you'd prefer to keep the archived Atom grammar. This issue has been open for years and I'm starting to understand why.

@lildude
Copy link
Member

lildude commented Feb 9, 2026

So this wasn't "not following instructions". The script cannot handle this grammar without the data.go fix, everything else in the diff is exactly what the script produces.

I can close this PR if you'd prefer to keep the archived Atom grammar. This issue has been open for years and I'm starting to understand why.

I have no problems with replacing the grammar with one that is maintained and makes things better (I'm assuming this is the case here) and I welcome the PR. However the PR isn't complete.

  1. The PR is missing the cached grammar license which should have been generated right at the end of the script/add-grammar:

bundle exec licensed cache -c vendor/licenses/config.yml

  1. The old grammar submodule hasn't been removed. The script should have removed it:

linguist/script/add-grammar

Lines 123 to 126 in e51c227

log "Deregistering submodule: $replace"
git submodule deinit "$replace"
git rm -rf "$replace"
rm -rf ".git/modules/$replace"

  1. The submodules file hasn't been sorted. Once again, the script does this.

  2. As a result of the above, the grammars.yaml changes don't look right. As you're removing the old grammar, it's entry should be completely removed from this file too, but it hasn't been.

  3. You have chosen not to use our PR template. Why? This is a requirement for ALL PRs and is why we highlight it in the CONTRIBUTING.md file. This is a very early detector that highlights if instructions have been followed.

The first four are probably due to things being in an inconsistent state due to the failures that necessitated the changes to the data.go file. Ideally we shouldn't be making changes like this as we expect the grammars to be in the exact form they are when used by the editors but I understand the need for it now.

The first four issues would also have been picked up by the tests.

@Chocapikk Chocapikk force-pushed the replace-shell-grammar-with-better-shell-syntax branch from 41a5e19 to e4239fa Compare February 9, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants