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

Simplify class and module grammar for more consistency #2242

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

vinistock
Copy link
Member

Motivation

One tiny thing that bothered me with the syntax highlighting is that our grammar incorrectly considered the :: separator as part of a class/module declaration, highlighting the entire thing in the same colour. This PR fixes that by simplifying the grammar regex.

Implementation

I simplified the regex in the grammar. There are things it was trying to account for that were unnecessary. For example, it tried to handle any arbitrary number of line breaks between the class keyword and the constant name (which TIL is valid Ruby). That's not necessary because semantic highlighting will catch that constant for us.

Another example is that it included trying to parse a parent class for modules, which is not valid Ruby module Foo < Bar.

We also don't need to account for syntax errors since those are already provided by the server. The summary is: we can make these regexes much simpler and easier to understand.

The way the grammar works is:

  1. Write a regex in the match attribute
  2. Define what highlight token should be assigned to each regex capture group

@vinistock vinistock added bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes labels Jul 2, 2024
@vinistock vinistock self-assigned this Jul 2, 2024
@vinistock vinistock requested a review from a team as a code owner July 2, 2024 15:35
@vinistock vinistock requested review from andyw8 and st0012 July 2, 2024 15:35
@andyw8
Copy link
Contributor

andyw8 commented Jul 3, 2024

What do you think about adding some tests (in Ruby) which read the JSON, extract the regexps, and perform some checks?

@vinistock
Copy link
Member Author

@andyw8 I think that's a great idea and necessary in the long run to maintain these grammar files in a healthy way. The Ruby one is huge and understanding all regexes is no easy task.

However, I'd prefer leaving that to a separate task. VS Code doesn't provide an easy way to test grammar files as far as I know. We will need to explore building a test framework that applies the grammar rules to a string of Ruby code and then allows us to assert the token types for every position. It's bound to take a little bit of effort to get right. I created #2249 so that we don't forget.

}
},
"match": "(?x)\n^\\s*(class)\\s+\n(\n (\n [.a-zA-Z0-9_:]+\n (\\s*(<)\\s*[.a-zA-Z0-9_:]+)? # class A < B\n )\n |\n ((<<)\\s*[.a-zA-Z0-9_:]+) # class << C\n)",
"comment": "class Namespace::ClassName < OtherNamespace::OtherClassName",
"match": "(class)\\s*(([a-zA-Z0-9_]+)((::)[a-zA-Z0-9_]+)*)\\s*((<)\\s*(([a-zA-Z0-9_]+)((::)[a-zA-Z0-9_]+)*))?",
Copy link
Contributor

@andyw8 andyw8 Jul 5, 2024

Choose a reason for hiding this comment

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

For illustration:

s = "class Namespace::ClassName < OtherNamespace::OtherClassName"
re_old = "(?x)\n^\\s*(class)\\s+\n(\n  (\n    [.a-zA-Z0-9_:]+\n    (\\s*(<)\\s*[.a-zA-Z0-9_:]+)?   # class A < B\n 
 )\n  |\n  ((<<)\\s*[.a-zA-Z0-9_:]+)         # class << C\n)"
s.match(re_old)
=> 
#<MatchData
 "class Namespace::ClassName < OtherNamespace::OtherClassName"
 1:"class"
 2:"Namespace::ClassName < OtherNamespace::OtherClassName"
 3:"Namespace::ClassName < OtherNamespace::OtherClassName"
 4:" < OtherNamespace::OtherClassName"
 5:"<"
 6:nil
 7:nil>

re_new = "(class)\\s*(([a-zA-Z0-9_]+)((::)[a-zA-Z0-9_]+)*)\\s*((<)\\s*(([a-zA-Z0-9_]+)((::)[a-zA-Z0-9_]+)*))?"
s.match(re_new)
=> 
#<MatchData
 "class Namespace::ClassName < OtherNamespace::OtherClassName"
 1:"class"
 2:"Namespace::ClassName"
 3:"Namespace"
 4:"::ClassName"
 5:"::"
 6:"< OtherNamespace::OtherClassName"
 7:"<"
 8:"OtherNamespace::OtherClassName"
 9:"OtherNamespace"
 10:"::OtherClassName"
 11:"::">

@vinistock vinistock force-pushed the vs/simplify_class_and_module_grammar branch from a404af1 to b7b2961 Compare July 5, 2024 17:35
@vinistock vinistock enabled auto-merge (squash) July 5, 2024 17:35
@vinistock vinistock merged commit 3df142c into main Jul 5, 2024
34 checks passed
@vinistock vinistock deleted the vs/simplify_class_and_module_grammar branch July 5, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants