Replace Vue Grammar by yuichkun · Pull Request #7086 · github-linguist/linguist · GitHub
Skip to content

Replace Vue Grammar#7086

Merged
lildude merged 3 commits into
github-linguist:mainfrom
yuichkun:replace-vue-grammar
Nov 25, 2024
Merged

Replace Vue Grammar#7086
lildude merged 3 commits into
github-linguist:mainfrom
yuichkun:replace-vue-grammar

Conversation

@yuichkun

@yuichkun yuichkun commented Oct 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR replaces the Vue.js grammar vendor from vuejs/vue-syntax-highlight to vuejs/language-tools.
The grammar file is located at https://github.com/vuejs/language-tools/blob/master/extensions/vscode/syntaxes/vue.tmLanguage.json

Description

Background:
There have been some issues stating that Vue's syntax breaks when certain newly introduced TypeScript syntax are present, such as optional chaining operator (?).

Related Issues:

Fix:

Since vuejs/vue-syntax-highlight has long been not updated (last commit is 4 years ago), and also there's even a collaborator of vuejs/language-tools suggesting linguist should switch Vue's grammar vendor to vuejs/language-tools (see this comment), here's the PR.

I'm not exactly sure if this really fixes all the problems, but I followed the CONTRIBUTING.md and just ran the following:

./script/add-grammar --replace vue-syntax-highlight https://github.com/vuejs/language-tools/blob/mast
er/extensions/vscode/syntaxes/vue.tmLanguage.json

In the script log, I found the following warnings:

5 warnings found when compiling new grammar 'repository `vendor/grammars/language-tools` (from https://github.com/vuejs/language-tools.git)':
- Unknown keys in grammar: `source.vue` (in `extensions/vscode/syntaxes/vue.tmLanguage.json`) contains invalid keys (`Repository[vue-directives-original].Patterns[0].1`, `Repository[vue-directives-original].Patterns[0].2`)
- Missing include in grammar: `source.vue` (in `extensions/vscode/syntaxes/vue.tmLanguage.json`) attempts to include `source.js.jsx` but the scope cannot be found
- Missing include in grammar: `source.vue` (in `extensions/vscode/syntaxes/vue.tmLanguage.json`) attempts to include `source.json.comments` but the scope cannot be found
- Missing include in grammar: `source.vue` (in `extensions/vscode/syntaxes/vue.tmLanguage.json`) attempts to include `source.json5` but the scope cannot be found
- Missing include in grammar: `source.vue` (in `extensions/vscode/syntaxes/vue.tmLanguage.json`) attempts to include `text.html.derivative` but the scope cannot be found

These warnings are not fatal, but may mean the syntax highlighting on GitHub.com may not be as expected.

Is this fine?

Checklist:

yuichkun
yuichkun commented Oct 12, 2024
View reviewed changes
Comment thread grammars.yml
@yuichkun yuichkun marked this pull request as ready for review October 12, 2024 19:47
@yuichkun yuichkun requested a review from a team as a code owner October 12, 2024 19:47
@lildude

lildude commented Oct 14, 2024

Copy link
Copy Markdown
Member

lildude
lildude reviewed Oct 14, 2024
View reviewed changes
Comment thread vendor/README.md
@yuichkun

yuichkun commented Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@lildude

The CI was failing with the following error:

  1) Failure:
TestLanguage#test_all_languages_have_grammars [test/test_language.rb:361]:
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.
Vue text.html.vue

And I assumed that I have to set tm_scope to source.vue in lib/linguist/languages.yml, so that it corresponds to the scope name specified in the new repo here.
So I fixed it so in ecb907d

Am I in the right direction?

Thank you for taking your time to review

@lildude

lildude commented Oct 15, 2024

Copy link
Copy Markdown
Member

lildude
lildude approved these changes Oct 15, 2024
View reviewed changes

@lildude lildude left a comment

Copy link
Copy Markdown
Member

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.

This was referenced Oct 20, 2024
Open
Closed
@lildude lildude added this pull request to the merge queue Nov 25, 2024
Merged via the queue into github-linguist:main with commit 8dac458 Nov 25, 2024
@matthew-white matthew-white mentioned this pull request Dec 5, 2024
Closed
This was referenced Jan 7, 2025
Open
Closed
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lildude lildude lildude approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yuichkun @lildude