-
Notifications
You must be signed in to change notification settings - Fork 74
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 new syntax patterns for dune(-project) files #1391
Conversation
Hi, this is my first time contributing here, sorry if I did something wrong. On a side note, I took note of some things while doing this, concerning the syntax highlighting and the documentation:
I hope this is helpful. Let me know if I should change something or if I missed something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the delay. The changes look good.
To fix the failing check, you need to add a change log entry to the CHANGELOG.md
file.
To address your questions:
-
I think most of the stanzas are covered now, with the exception of the stanzas from the config section, which I do not know if they're in a separate file or not.
The documentation mentions that
config
stanzas can be used indune-workspace
. That means the syntax patterns need to be added todune-workspace.json
. -
I was not sure if I was allowed to make a separate rule in the file's repository for both the normal
menhir
stanza and themenhir
stanza inenv
. I can add it if it's okay.That's a good idea.
-
The documentation for the
subst
stanza seems to be incorrect, I think. It says that thesubst
stanza takes a<bool>
as an argument, but it actually takes<enabled|disabled>
. For example, thewarnings
stanza takes<enabled|disabled>
and is documented as such. If you want, I can make a PR to fix this.I'm not very familiar with these specifics, but I think you're right. You can make a PR to correct the documentation if you wish.
-
Another thing that I noticed is that the different TextMate grammar files have different formatting styles. This isn't that big of a deal, but it's something that I noticed.
Our formatter for JSON files, Prettier, tends to preserve newlines. There may be some settings to make the formatting more consistent, but I agree with you that it's not a big deal.
-
Should
external_variant
be added to the syntax highlighting? I didn't add it as it was an experimental feature removed in version 2.6 of dune, and there's no documentation for it in the dune docs anymore.I agree with your decision. There's little point in highlighting a removed feature, especially one that was experimental.
-
I noticed that there is a
variable.declaration.other.dune
name and avariable.other.declaration.dune
name in the syntax highlighting files. I don't know if this is intentional or not, and I do not know if there is a difference between the two. I tried both of them and they both seem to work the same way.I don't think it was intentional. I believe it's supposed to be
variable.other.declaration.dune
since that's closer to what other languages use. -
For the
install
stanza, the documentation only mentions 3 fields (files, section, package), but the syntax highlighting file is highlighting way more fields than that. I think there may have been a mistake with theexecutable
fields, as there's a mention of theinstall
stanza in the documentation for theexecutable
stanza followed by the fields for theexecutable
stanza. I'm not sure if this is a mistake or not, but I mentioned it just in case.There does seem to be some mistake here. I don't think it was a confusion with the
executable
fields though. Theinstall
stanza documentation includes a table that has most of the current fields. The problem is that the list in the table is supposed to represent values for thesection
field, not field names like our current interpretation.
Let me know if you have any further questions.
Hi, thank you for the feedback! |
Sorry for the delay, got caught up with my studies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. I found one typo. Everything else looks fine.
Resolves #1180