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 Dune grammar and add more filenames #7126

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

huwaireb
Copy link

@huwaireb huwaireb commented Nov 12, 2024

Description

Migrates to the official OCaml vscode extension's TextMate grammar for Dune. And adds dune and dune-workspace to the files to be recognized.

Checklist:

@huwaireb huwaireb requested a review from a team as a code owner November 12, 2024 12:43
@huwaireb
Copy link
Author

huwaireb commented Nov 12, 2024

There's one non-blocking issue with this update,

vscode-ocaml-platform has three textmate grammars for dune:

  • source.dune
  • source.dune-project
  • source.dune-workspace

source.dune-project and source.dune-workspace include source.dune#general. However, they don't share source.dune#stanzas, as each has their own set of supported stanzas, e.g source.dune-project#stanzas. Given linguist offers no way to use multiple grammars per language, the only way is to merge all possible stanzas into a single tmLanguage.json grammar and put it inside another repo, or put it in vscode-ocaml-platform and make a new source that is source.dune-all for example.

cc @smorimoto

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 update the template to fill in the appropriate section for adding the other filenames, You need to fill out all that apply and this is not a simple grammar switch with these additions.

Please note, if any of these new files do not meet our usage requirements, this PR will be blocked until they do meet our requirements or they are removed from this PR. Accordingly, you might want to consider splitting this PR into two.

@lildude
Copy link
Member

lildude commented Nov 12, 2024

  • source.dune

This will be the only grammar used and it will be applied to all files identified as Dune as per the tm_scope: source.dune in the language.yml. Multiple grammars can no apply to the same language. The same grammar can however apply to multiple languages.

@huwaireb
Copy link
Author

huwaireb commented Nov 12, 2024

Please update the template to fill in the appropriate section for adding the other filenames, You need to fill out all that apply and this is not a simple grammar switch with these additions.

I've updated the PR to reflect.

Please note, if any of these new files do not meet our usage requirements, this PR will be blocked until they do meet our requirements or they are removed from this PR. Accordingly, you might want to consider splitting this PR into two.

They meet the requirements, except for dune-workspace.dev which only has 75 results. I can remove that, but most use of dune-workspace will revolve around it being scoped like that.

@huwaireb huwaireb requested a review from lildude November 12, 2024 18:46
@lildude
Copy link
Member

lildude commented Nov 25, 2024

They meet the requirements, except for dune-workspace.dev which only has 75 results. I can remove that, but most use of dune-workspace will revolve around it being scoped like that.

You'll need to remove this filename if you want this PR merged.

@lildude lildude changed the title Update the grammar used for Dune Replace Dune grammar and add more filenames Nov 25, 2024
Includes `dune` and `dune-workspace`.
@huwaireb
Copy link
Author

You'll need to remove this filename if you want this PR merged.

I've removed references to dune-workspace.dev

@lildude
Copy link
Member

lildude commented Nov 25, 2024

Looks like you might not have used the script/add-grammar command to replace the grammar.

@huwaireb
Copy link
Author

huwaireb commented Nov 25, 2024

Looks like you might not have used the script/add-grammar command to replace the grammar.

Mm, i'm pretty sure i had, but it was weird. It didn't generate some files, i had to replace them manually.

Looks like i missed updating gitmodules, one second.

@huwaireb
Copy link
Author

Should be okay now? Just missed deleting the actual submodule.

@lildude
Copy link
Member

lildude commented Nov 25, 2024

Please use the script. It correctly orders things in various files that are automatically maintained by it. It also validates the grammar files.

This should do the trick:

script/add-grammar --replace vendor/grammars/vscode-ocaml-platform https://github.com/ocamllabs/vscode-ocaml-platform.git

@huwaireb
Copy link
Author

Please use the script. It correctly orders things in various files that are automatically maintained by it. It also validates the grammar files.

This should do the trick:

script/add-grammar --replace vendor/grammars/vscode-ocaml-platform https://github.com/ocamllabs/vscode-ocaml-platform.git
Screenshot 2024-11-25 at 21 22 40

That did not do the trick

@huwaireb
Copy link
Author

Screenshot 2024-11-25 at 21 25 14

@huwaireb
Copy link
Author

huwaireb commented Nov 25, 2024

Ah there were errors in upstream's dune grammar,

Screenshot 2024-11-25 at 21 35 40

will sort it out

Not sure how I didn't get this error before, but sure.

@huwaireb
Copy link
Author

huwaireb commented Nov 25, 2024

@lildude, quick question relating to the previous non-blocking issue I mentioned.

If I have a new tmLanguage.json syntax file that includes other patterns as follows

{ 
"name": "dune-all",
"scopeName": "source.dune-all",
"fileTypes": ["dune", "dune-project", "dune-workspace"],
"patterns": [{ "include": "source.dune#general" }, { "include": "source.dune#stanzas" }, { "include": "source.dune-project#stanzas" }, { "include": "source.dune-workspace#stanzas" }],
}

would that work with linguist? These "sources" come from the same submodule.

It's just to reduce maintainership burden, otherwise we can have a script that merges all three into a single file.

@lildude
Copy link
Member

lildude commented Nov 27, 2024

would that work with linguist? These "sources" come from the same submodule.

Yes, I believe the syntax highlighter will successfully pull those in as we have other grammars that use includes without any reports of problem. @Alhadis is more of an expert about writing grammars so can confirm from his experience.

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 28, 2024

would that work with linguist? These "sources" come from the same submodule.

@huwaireb Yes. In fact, the largest grammar repository I maintain, language-etc, makes extensive use of include: "etc#…" directives to drastically cut down on repeated/mostly-identical pattern rules. Which is particularly pertinent for a repository that mostly contains grammars to highlight assorted config files and data formats. 😉

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.

3 participants