-
Notifications
You must be signed in to change notification settings - Fork 58
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
should go after endCaptures
but before patterns
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be made a 0th capture
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make the overarhcing name or the hierarchy less obvious?
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.
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