Dune syntax highlighting fix #742

SaySayo · 2021-10-16T17:12:13Z

Fixed the dune syntax by rewriting the dune.json in a similar fashion to the dune-project.json and dune-workspace.json syntaxes (#106). @mnxn please review

mnxn · 2021-10-19T17:23:54Z

A couple of things I've noticed so far:

Missing executable fields
Missing lint
Overuse of variable.other.declaration.dune. I only used this scope name when something is being declared. In this example, I would only expect public_name and name to have the variable.other.declaration.dune highlighting.

@rgrinberg: do you know of any large dune files that could be used to verify the syntax highlighting?

SaySayo · 2021-10-19T17:49:14Z

I used this dune docs(https://dune.readthedocs.io/en/stable/dune-files.html#dune) which I assumed is the official doc for referencing of the stanzas and It didn't contain lint and most of the executables you just listed, hence why they weren't included. Can you please recommend a more accurate doc that contains all the executables so I can update it.

mnxn · 2021-10-19T17:56:13Z

Hmm. You are right about lint, I don't know why it isn't documented. That is the official documentation.

The executable fields are all listed though: https://dune.readthedocs.io/en/stable/dune-files.html#executable
Scroll down to where it describes the <optional-fields>.

SaySayo · 2021-10-19T17:58:00Z

Also, I assumed the declarations were variables because the available syntax highlight options with a foreground color were either constants, variables, keywords, or strings. And I was certain they weren't constants or keywords. What do you recommend I use instead so I can make necessary changes

mnxn · 2021-10-19T17:59:59Z

either constants, variables, keywords, or strings.

If it's not one of those, then don't include the contentName field, it's optional.

SaySayo · 2021-10-19T18:03:28Z

syntaxes/dune.json

+              "1": { "name": "keyword.language.dune" }
+            },
+            "patterns": [{
+              "comment": "name/public_name/package/libraries/link_flags/link_deps/modules/root_module/preprocess/preprocessor_deps/modules_without_implementation/optional/enabled_if/promote/foreign_stubs/foreign_archives/forbidden_libraries/embed_in_plugin_libraries",


@mnxn The executable optional fields were listed here. I'll go through it again to see why they weren't highlighted in your tests

tmattio · 2021-10-19T18:06:10Z

I used this dune docs(https://dune.readthedocs.io/en/stable/dune-files.html#dune) which I assumed is the official doc for referencing of the stanzas and It didn't contain lint

Feel free to open an issue on dune (https://github.com/ocaml/dune) if you notice undocumented fields @SaySayo.

SaySayo · 2021-10-19T18:06:42Z

If it's not one of those, then don't include the contentName field, it's optional.

Noted. I'll make the necessary changes.

SaySayo · 2021-10-19T18:10:45Z

Overuse of variable.other.declaration.dune. I only used this scope name when something is being declared. In this example, I would only expect public_name and name to have the variable.other.declaration.dune highlighting.

To be clear, Do you mean js_of_ocaml, mode, pps etc aren't variables and I should remove the variable highlighting on them?

mnxn · 2021-10-19T18:12:13Z

To be clear, Do you mean js_of_ocaml, mode, pps etc aren't variables and I should remove the variable highlighting on them?

Yes, that's right.

syntaxes/dune.json

mnxn · 2021-10-25T23:18:13Z

syntaxes/dune.json

+            },
+            "patterns": [{
+              "comment": "name/public_name/package/libraries/link_flags/link_deps/modules/root_module/preprocess/preprocessor_deps/modules_without_implementation/optional/enabled_if/promote/foreign_stubs/foreign_archives/forbidden_libraries/embed_in_plugin_libraries",
+              "begin": "\\([[:space:]]*(name|public_name|package/libraries|link_flags|link_deps|modules|root_module|preprocess|preprocessor_deps|modules_without_implementation|optional|enabled_if|promote|foreign_stubs|foreign_archives|forbidden_libraries|embed_in_plugin_libraries)\\b",


I think the / in package/libraries is a typo here. It should be package|libraries.

Also name|public_name should be separated into it's own rule with "contentName": "variable.other.declaration.dune" the same way as in a library.

modes is missing: https://dune.readthedocs.io/en/stable/dune-files.html#executables

pps in preprocess needs to be highlighted the same as a library:

mnxn · 2021-10-25T23:23:30Z

syntaxes/dune.json

+          },
+          {
+            "comment": "executable/executables",
+            "begin": "\\([[:space:]]*(executables*)\\b",


The * makes the rule match executable followed by any number of s's.

It should be:

Suggested change

"begin": "\\([[:space:]]*(executables*)\\b",

"begin": "\\([[:space:]]*(executable|executables)\\b",

Also executables needs to support names and public_names: https://dune.readthedocs.io/en/stable/dune-files.html#executables

syntaxes/dune.json

rgrinberg · 2021-11-21T17:41:01Z

@SaySayo do you plan to work on this PR?

SaySayo · 2021-11-21T18:55:55Z

Yes I do. I made the changes @mnxn requested and I'm waiting on a review.

mnxn · 2021-11-25T00:03:46Z

Sorry for not looking at this earlier.

I tested this syntax with some example dune files from the dune repo and there are still many unrecognized fields:

Examples

I think there is still a lot of work left until this syntax is complete. The core issue is that there are too many unrelated fields in certain rules: like this line for example. Some of the fields can have extra options or specific input, so it doesn't make sense to combine them all.

What could make this easier to work on is an example dune file that covers most of the dune syntax. That way you would just need to scroll through that file to see what is being highlighted correctly or not. Unfortunately, I haven't found anything like that so far.

I don't want to merge this PR until the syntax is complete. You are free to keep working on this if you want, but I can't guarantee to keep up with code reviews. I've had much less time for reviews recently and this is a very big PR.

SaySayo · 2021-11-25T09:45:00Z

I'll keep working on this PR till it fixes all the issues you mentioned, might take me a couple weeks as I have less time to work on it. I'll ping you when I think its fully ready for a review so as to reduce the amount of times you have to run a review on this PR. In the meantime I'll convert this PR to a draft since it's not ready to be merged while I continue to work on it.

mnxn · 2021-11-25T20:14:17Z

Sounds good. Thanks.

tmattio · 2021-12-07T14:57:46Z

@mnxn We're currently reviewing the PR with @SaySayo and are not sure how to address your comment about the unrecognized fields.

You previously mentioned:

Overuse of variable.other.declaration.dune. I only used this scope name when something is being declared. In this example, I would only expect public_name and name to have the variable.other.declaration.dune highlighting.

And advised that some fields should not be highlighted.

In your last comment, though, you're mentioning that

I tested this syntax with some example dune files from the dune repo and there are still many unrecognized fields:

IIUC, these were previously highlighted, but @SaySayo removed it after your comment. What would you recommend we do? Should we create a new scope for these? Do you have any guidelines you're following to decide what is a variable.other.declaration.dune and what is not?

Personally, I think that's fine if all the fields are highlighted, but we'll follow your recommendation here.

mnxn · 2021-12-07T21:11:29Z

Sorry, I will try to use more specific language.

From now on I will refer to field names and field values. In the snippet (a b), a is the field name and b is the field value.

What I previously mentioned was that only the field values for public_name and name should use variable.other.declaration.dune. What I forgot to say back that then is that field values for the plurals, public_names and names, should also be variable.other.declaration.dune.

So only field values for public_name, public_names, name, and names should use variable.other.declaration.dune.

My most recent screenshots were mostly to point out the missing field name highlighting. We should aim to highlight all valid field names in the appropriate contexts.

I'll go over the specific changes in those screenshots I would expect. This time I will use the built-in Dark+ theme.

i. names should be highlighted with keyword.language.dune
ii. foo should be highlighted with variable.other.declaration.dune
iii. libraries should be highlighted with keyword.language.dune
i. gh637 should be highlighted with variable.other.declaration.dune
ii. package should be highlighted with keyword.language.dune
iii. libraries should be highlighted with keyword.language.dune
iv. deps should be highlighted with keyword.language.dune
i. language should be highlighted with keyword.language.dune
ii. names should be highlighted with keyword.language.dune
i. with-stdout-to should be highlighted with entity.name.function.action.dune
ii. run should be highlighted with entity.name.function.action.dune
i. backend should be highlighted with keyword.language.dune
ii. executable should be highlighted with keyword.language.dune
iii. flags should be highlighted with keyword.language.dune

SaySayo · 2021-12-08T19:38:41Z

Hello @mnxn I updated the dune syntax to address all the things you mentioned. Works well locally. Please review when you can and let me know if there's any required changes. Thank you!

tmattio · 2021-12-08T20:55:44Z

@mnxn Thanks a lot for the very detailed and comprehensive answer, it's very helpful 🙂

@SaySayo I'd like to test the PR with some more dune files to see if we can spot other issues before asking @mnxn to review again if that's ok with you. I'll ping here if everything looks good after testing.

SaySayo · 2021-12-08T20:58:20Z

Yes that's fine @tmattio. Thank you for your time!

tmattio · 2022-01-26T15:34:14Z

@mnxn I completed an initial review of the PR and I believe it is ready for you to review as well.

I noticed some issues that should be addressed:

We are forgetting to include #general in some places, which breaks the syntax highlighting for comments in some stanzas, among other things
the env stanza fields are not highlighted
include_subdirs values are not highlighted
The variable highlighting is too general, for instance, the extension .exe is highlighted in %{exe:my_test_program.exe}.

However, fixing these shouldn't require massive changes, so it's not conflicting with additional reviews.

mnxn

Sorry for the delay in review.

This looks great! It's a huge improvement over the current syntax. Thank you for all your hard work!

Before merging, the .DS_Store file needs to be removed and the branch needs to be rebased to fix the CHANGES.md conflict.

mnxn · 2022-02-01T19:22:36Z

The variable highlighting is too general

@tmattio:
The variable highlighting is actually the same across syntaxes for dune, dune-project, and dune-workspace. I think it should be fixed in another PR so this PR stays isolated.

tmattio · 2022-02-01T21:17:25Z

This is a huge undertaking, thank you so much for all your work on this @SaySayo. Congratulations on getting the PR merged 🎉

Thanks a lot for all your time, patience, and support to get this ready to merge @mnxn. Truly appreciate it ❤️

SaySayo · 2022-02-01T21:27:06Z

I'm so excited to see the changes finally merged! Glad I could make a difference. Thank you so much for your extremely patient reviews and solving through all the blockers with me @tmattio @mnxn @gs0510. I really appreciate it 🙂🙏

mnxn self-requested a review October 16, 2021 17:14

SaySayo commented Oct 19, 2021

View reviewed changes

mnxn reviewed Oct 22, 2021

View reviewed changes

syntaxes/dune.json Outdated Show resolved Hide resolved

mnxn reviewed Oct 25, 2021

View reviewed changes

syntaxes/dune.json Outdated Show resolved Hide resolved

mnxn reviewed Oct 25, 2021

View reviewed changes

syntaxes/dune.json Outdated Show resolved Hide resolved

mnxn reviewed Oct 25, 2021

View reviewed changes

syntaxes/dune.json Outdated Show resolved Hide resolved

SaySayo marked this pull request as draft November 25, 2021 09:45

SaySayo closed this Jan 12, 2022

SaySayo deleted the sayo_branch branch January 12, 2022 13:05

SaySayo restored the sayo_branch branch January 15, 2022 17:47

SaySayo reopened this Jan 15, 2022

SaySayo marked this pull request as ready for review January 19, 2022 21:50

mnxn approved these changes Feb 1, 2022

View reviewed changes

SaySayo force-pushed the sayo_branch branch 2 times, most recently from 930a815 to ca44d22 Compare February 1, 2022 20:24

SaySayo and others added 3 commits February 1, 2022 22:03

Dune syntax highlighting fix

faaf7ad

Remove TODO

a00aa6a

Add description to Changelog

27141d7

tmattio force-pushed the sayo_branch branch from ca44d22 to 27141d7 Compare February 1, 2022 21:10

Fix changelog typos

85b5e11

tmattio merged commit 042ed95 into ocamllabs:master Feb 1, 2022

tmattio mentioned this pull request Feb 6, 2022

Improve dune highlighting #106

Closed

mnxn mentioned this pull request Feb 15, 2022

Unify and improve general dune syntax #872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dune syntax highlighting fix #742

Dune syntax highlighting fix #742

SaySayo commented Oct 16, 2021

mnxn commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

SaySayo Oct 19, 2021

tmattio commented Oct 19, 2021

SaySayo commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

mnxn Oct 25, 2021

mnxn Oct 25, 2021

mnxn Oct 25, 2021

mnxn Oct 25, 2021 •

edited

Loading

mnxn Oct 25, 2021

rgrinberg commented Nov 21, 2021

SaySayo commented Nov 21, 2021

mnxn commented Nov 25, 2021

SaySayo commented Nov 25, 2021

mnxn commented Nov 25, 2021

tmattio commented Dec 7, 2021

mnxn commented Dec 7, 2021

SaySayo commented Dec 8, 2021

tmattio commented Dec 8, 2021 •

edited

Loading

SaySayo commented Dec 8, 2021

tmattio commented Jan 26, 2022

mnxn left a comment

mnxn commented Feb 1, 2022 •

edited

Loading

tmattio commented Feb 1, 2022

SaySayo commented Feb 1, 2022

	"begin": "\\([[:space:]](executables)\\b",
	"begin": "\\([[:space:]]*(executable\|executables)\\b",

Dune syntax highlighting fix #742

Dune syntax highlighting fix #742

Conversation

SaySayo commented Oct 16, 2021

mnxn commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

SaySayo Oct 19, 2021

Choose a reason for hiding this comment

tmattio commented Oct 19, 2021

SaySayo commented Oct 19, 2021

SaySayo commented Oct 19, 2021

mnxn commented Oct 19, 2021

mnxn Oct 25, 2021

Choose a reason for hiding this comment

mnxn Oct 25, 2021

Choose a reason for hiding this comment

mnxn Oct 25, 2021

Choose a reason for hiding this comment

mnxn Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

mnxn Oct 25, 2021

Choose a reason for hiding this comment

rgrinberg commented Nov 21, 2021

SaySayo commented Nov 21, 2021

mnxn commented Nov 25, 2021

SaySayo commented Nov 25, 2021

mnxn commented Nov 25, 2021

tmattio commented Dec 7, 2021

mnxn commented Dec 7, 2021

SaySayo commented Dec 8, 2021

tmattio commented Dec 8, 2021 • edited Loading

SaySayo commented Dec 8, 2021

tmattio commented Jan 26, 2022

mnxn left a comment

Choose a reason for hiding this comment

mnxn commented Feb 1, 2022 • edited Loading

tmattio commented Feb 1, 2022

SaySayo commented Feb 1, 2022

mnxn Oct 25, 2021 •

edited

Loading

tmattio commented Dec 8, 2021 •

edited

Loading

mnxn commented Feb 1, 2022 •

edited

Loading