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

Add Glimmer JS language #6578

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Add Glimmer JS language #6578

merged 5 commits into from
Dec 6, 2023

Conversation

gilest
Copy link
Contributor

@gilest gilest commented Oct 12, 2023

Description

Adds support for Glimmer.js which will be the component authoring format of the next Edition of Ember.js.

Checklist:

@gilest gilest requested a review from a team as a code owner October 12, 2023 22:02
@gilest gilest mentioned this pull request Oct 12, 2023
6 tasks
@gilest
Copy link
Contributor Author

gilest commented Oct 18, 2023

@lildude would you mind running the CI for me? Want to make sure I didn't make a mistake this time 😅

Never mind – got it running in my fork ✅

Believe we are now meeting the usage requirements, too 🎉

@NullVoxPopuli
Copy link

really excited for this! looks like we're no longer "Pending Popularity"

@lildude
Copy link
Member

lildude commented Oct 25, 2023

really excited for this! looks like we're no longer "Pending Popularity"

Not quite. Excluding forks drops things down to ~1.8k.

@NullVoxPopuli
Copy link

NullVoxPopuli commented Oct 27, 2023

Not quite. Excluding forks drops things down to ~1.8k.

can we include markdown which references gjs? (that's at 2k 🥳 )

update: we're over 2k non-markdown files now :D

@wagenet
Copy link
Contributor

wagenet commented Nov 11, 2023

@lildude It looks like gjs without markdown and excluding forks is now at 2k.

grammars.yml Show resolved Hide resolved
@lildude lildude added this pull request to the merge queue Dec 6, 2023
Merged via the queue into github-linguist:master with commit a4e2bf9 Dec 6, 2023
5 checks passed
@lildude
Copy link
Member

lildude commented Dec 6, 2023

Heads up: this language won't be using the most up-to-date version of the grammar when it is released to GitHub as a duplicate scope has been added to that repo since this PR was created.

- [ ] repository `vendor/grammars/Handlebars` (from https://github.com/daaain/Handlebars) (1 errors)
  - Duplicate scope in repository: scope `text.html.handlebars` was already defined in repository `vendor/grammars/vsc-ember-syntax` (from https://github.com/lifeart/vsc-ember-syntax.git)

vendor/grammars/Handlebars has priority as it was here first and is dedicated to the language.

@lildude
Copy link
Member

lildude commented Dec 6, 2023

Actually, now I look at this more closely, it appears this grammar was added with that conflict already in place suggesting script/add-grammars wasn't used or the addition was forced and the error ignored. Reverting this PR.

Please resubmit when the issue with the upstream grammar has been fixed.

lildude added a commit that referenced this pull request Dec 6, 2023
lildude added a commit that referenced this pull request Dec 6, 2023
Revert "Add Glimmer JS language (#6578)"

This reverts commit a4e2bf9.
@NullVoxPopuli
Copy link

NullVoxPopuli commented Dec 6, 2023

Is there a way to tell vsc-ember-syntax to take precedence over daaain/Handlebars?

afaict, The upstream grammar is correct for ember users, and no ember user should ever be using daaain/Handlebars (only a file extension is shared, and nothing more)

For ember projects using the .hbs extension, "Handlebars" is just the wrong syntax. There are specific file differences we could query for in package/workspace boundaries to determine if a project/package/workspace is an ember project or not, but does linguist respect that sort of querying?

@gilest
Copy link
Contributor Author

gilest commented Dec 6, 2023

Actually, now I look at this more closely, it appears this grammar was added with that conflict already in place suggesting script/add-grammars wasn't used or the addition was forced and the error ignored. Reverting this PR.

Please resubmit when the issue with the upstream grammar has been fixed.

Oh, sorry about that 😓 . I used script/add-grammars but then manually removed text.html.handlebars from vsc-ember-syntax in grammars.yml to avoid conflicting with the existing Handlebars grammar. Figured since the CI passed that was ok...

We discussed this in the public feedback PR before this one was raised.

Is there any way to proceed without removing handlebars from the upstream grammar? It's needed for in-editor support of Ember-flavoured handlebars. But ideally it's ignored in Linguist... 😅

@lildude
Copy link
Member

lildude commented Dec 7, 2023

Is there a way to tell vsc-ember-syntax to take precedence over daaain/Handlebars?

Not really. Linguist, more specially the highlighting engine, only has support for one grammar scope per language and it doesn't have a mechanism for overriding. Generally grammars inherit and extend upon other grammars using a different scope name if they want something to behave differently from the original grammar.

The closest option to forcing is by replacing the current Handlerbars grammar with that supplied by vsc-ember-syntax. This comes with the consequence that that project will now be responsible for the syntax highlighting of all handlebar files, not just Ember users.

afaict, The upstream grammar is correct for ember users, and no ember user should ever be using daaain/Handlebars (only a file extension is shared, and nothing more)

For ember projects using the .hbs extension, "Handlebars" is just the wrong syntax.

And there's the problem. If the file and syntax is not handlerbars, the grammar should not be using the same scope name. It should be using its own scope and either completely defining everything it supports or inheriting from the handlebars grammar and overruling the differences. This is common practice.

There are specific file differences we could query for in package/workspace boundaries to determine if a project/package/workspace is an ember project or not, but does linguist respect that sort of querying?

Not unless there's something in the files themselves that is unique to ember. Linguist looks at files in isolation (think gists) to try and determine the language which is then fed to the highlighting engine.

If the files are definitely not handlebars and should have their own grammar, then they can be added to linguist as their own language but the grammar will still need to use a unique scope name.

@lildude
Copy link
Member

lildude commented Dec 7, 2023

Is there any way to proceed without removing handlebars from the upstream grammar? It's needed for in-editor support of Ember-flavoured handlebars. But ideally it's ignored in Linguist... 😅

Yes, however that's just painting over the problem and incorrect behaviour I just highlighted above.

@gilest gilest mentioned this pull request Dec 7, 2023
5 tasks
@gilest
Copy link
Contributor Author

gilest commented Dec 7, 2023

@lildude Appreciate your counsel on this.

We've updated the upstream grammar to correctly use a unique scope for "ember-flavoured" handlebars – text.html.ember-handlebars

Re-staged in #6630

@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 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.

5 participants