Highlight all soft modifiers #168
Conversation
camilaagw
commented
Dec 10, 2020
camilaagw
commented
| name: 'storage.modifier.other' | ||
| }, | ||
| { | ||
| match: '(?<=^|\\s)\\b(transparent|opaque|infix|open|inline)\\b(?=[a-z\\s]*\\b(def|val|var|given|type|class|trait|object|enum)\\b)', |
There was a problem hiding this comment.
Instead of allowing [a-z\\s]* between a soft keyword modifier and a keyword, I think we should aim to only highlight a chain of soft keywords followed by a keyword, something like ((transparent|opaque|infix|open|inline)\\b)+(?=\\b(def|val|var|given|type|class|trait|object|enum)) (might need some adjustment for allowing whitespace). With soft keywords, I think we should try to be as minimal as possible, only highlighting them as keywords if strictly necessary.
There was a problem hiding this comment.
Note that a soft modifier can be followed by a sequence of normal modifiers. [a-z\\s]* skips those modifiers to find the def|val|....
There was a problem hiding this comment.
The alternative would be to list all modifiers separated by spaces instead of [a-z\\s]*
| // ^^^^^^ storage.modifier.other | ||
|
|
||
| transparent badkeyword inline override def alternative | ||
| // ^^^^^^^^^^^ storage.modifier.other |
There was a problem hiding this comment.
Related to the previous comment: I don't think that we should have a test requiring that transparent is storage.modifier.other in this situation. The general rule of thumb that I've been applying is that when it's invalid Scala, it can be highlighted as anything; we shouldn't require specific highlightings for invalid Scala in our tests, because that could make future refactorings more difficult.
There was a problem hiding this comment.
This is just a consequence of the [a-z\\s]* above.
There was a problem hiding this comment.
This might be practical if we have
transparent inilne def alternative
In this case transparent and def are highlighted but not inilne which makes it trivial to find the typo.
There was a problem hiding this comment.
Okay, that's actually a quite convincing argument for keeping [a-z\\s]*. In that case, we can merge the PR.
