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

vscode: fix tmGrammar issues around non-controlflow keywords #6497

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Nov 7, 2020

Addresses some of the issues mentioned here: dustypomerleau/rust-syntax#5.
In sync with dustypomerleau/rust-syntax#6.

@dustypomerleau
Copy link
Contributor

bors r+

Happy with these changes. For those themes that make a storage/keyword distinction it adds consistency.

@bors
Copy link
Contributor

bors bot commented Nov 8, 2020

@bors bors bot merged commit 945900b into rust-lang:master Nov 8, 2020
@@ -180,7 +180,7 @@
"begin": "\\b(extern)\\s+(crate)",
"beginCaptures": {
"1": {
"name": "keyword.control.rust"
"name": "storage.type.rust"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just realized. This might be wrong. I think this should've been keyword.other.rust. The extern keyword doesn't signify any storage here.

Copy link
Contributor

@dustypomerleau dustypomerleau Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never been clear on precisely what constitutes storage in Textmate grammars. The official documentation states that storage should be used for 'things relating to "storage,"' which is a little bit circular for the uninitiated. @bjorn3 had specific opinions when I originally proposed the grammar, so perhaps could weigh in here. My impression is that storage applies to situations where memory is allocated, but there is a lot of gray area there. CC: @georgewfraser.

I'm happy to merge a reverting PR on these two scopes, but let's hold off for some consensus. As an aside, the grammar probably needs tests so that it doesn't become a brittle house of cards when others contribute. I'll have a look at implementing PanAeon's approach when things slow down on the issue front.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be storage.modifier. See https://macromates.com/manual/en/language_grammars

@@ -167,7 +167,7 @@
"match": "(mod)\\s+((?:r#(?!crate|[Ss]elf|super))?[a-z][A-Za-z0-9_]*)",
"captures": {
"1": {
"name": "keyword.control.rust"
"name": "storage.type.rust"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as extern. @dustypomerleau, @matklad?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, I think this should be storage.modifier

@cynecx cynecx deleted the vscode-fix-grammar2 branch January 2, 2021 19:21
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