Rewrite #at_rule_include #137
Conversation
|
In https://github.com/atom/language-sass/blob/master/grammars/scss.cson#L294 \\s*((@)include\\b)\\s*Is the first |
|
Nope. |
| @@ -291,27 +291,34 @@ | |||
| 'at_rule_include': | |||
| 'patterns': [ | |||
| { | |||
| 'begin': '\\s*((@)include\\b)\\s*' | |||
| 'name': 'meta.at-rule.include.scss' | |||
There was a problem hiding this comment.
name should go after endCaptures but before patterns.
There was a problem hiding this comment.
I noticed this in the surrounding code, but this ordering actually makes the code much harder to read. Do we really have to do that?
Every tutorial I read about creating Textmate bundles puts name on the top in their examples. I think this makes more sense: 'name' is just like a title of an article here—it tells you what the pattern is about, you wouldn't put the title of an article in the middle of everything; you often need to use the 'name' somewhere else, putting it in the middle makes it much harder to find, which is really bothersome when you try to do it in the surrounding code.
I know it's important to make your code consistent with the surrounding code; I always try as hard as I can to do it when contributing. But it just feels wrong in this case.
There was a problem hiding this comment.
We're currently discussing how to make the grammars the most readable in atom/language-php#137. If you have any suggestions you should definitely voice them over there as well.
|
@50Wliu Specs are also updated accordingly, anything else? |
| 'name': 'punctuation.definition.parameters.begin.bracket.round.scss' | ||
| 'end': '(\\))' | ||
| 'endCaptures': | ||
| '1': |
There was a problem hiding this comment.
This can be 0 instead and the capturing group removed.
|
Done. So for now, I just have to place |
| 'captures': | ||
| '1': | ||
| 'name': 'entity.name.function.scss' | ||
| 'name': 'meta.at-rule.include.scss' |
There was a problem hiding this comment.
This can be made a 0th capture
There was a problem hiding this comment.
This capture might be needed, otherwise the preceding space characters would be tokenized as part of the "entity name"; and + can't be used in lookaround.
Correct me if I'm wrong.
There was a problem hiding this comment.
I meant the overarching name, not the 1st capture. ie
'captures':
'0':
'name': 'meta.at-rule.include.scss'
'1':
'name': 'entity.name.function.scss'There was a problem hiding this comment.
Wouldn't it make the overarhcing name or the hierarchy less obvious?
There was a problem hiding this comment.
To me it doesn't because it's pretty well ingrained in my head that 0 = the whole match (also since it comes first). It just doesn't feel right to have both a captures and a name.
|
Two final comments. Then it just needs to be rebased. |
Fixes #132