Improve mixin scopes by dsifford · Pull Request #82 · atom/language-less

dsifford · 2017-10-04T20:23:24Z

Work In Progress

Description of the Change

Todo

add .bracket.round to the punctuation classes (punctuation.definition.parameters.end.bracket.round.less)
revert back to key-value from operator.assignment for colon separating parameter name and default value
fix missing .meta.mixin.less scopes in numeric default parameters.
lots and lots of tests

Resolves #81

winstliu · 2017-10-04T21:22:58Z

FYI I will be gone starting tomorrow until the 16th. So please feel free to create PRs for the other issues you wanted to tackle and I will try to review them after I get back.

dsifford · 2017-10-04T22:15:09Z

grammars/less.cson

+    'patterns': [
+      {
+        'match': ':'
+        'name': 'punctuation.separator.key-value.less'


@50Wliu Should this be punctuation.separator.key-value.css? Or is it incorrectly set here: https://github.com/atom/language-less/pull/82/files#diff-14c55924b6cf1fe75bf40519b9cddaccR317?

I think this can be kept as .less.

Should I change the other occurrence then?

The other one can be kept as .css because that one is the same in CSS, whereas for this one there is no function equivalent in CSS.

It's a bit confusing and is pretty ad-hoc. Hopefully in the future the endings can be standardized.

dsifford · 2017-10-05T01:52:51Z

@50Wliu Got all the current tests passing. I'll hold off on going any further for now until I can get confirmation from you that the structure looks acceptable.

Enjoy your vacation!

winstliu · 2017-10-16T11:10:57Z

grammars/less.cson

+    # http://lesscss.org/features/#mixin-guards-feature-guard-comparison-operators
+    'match': '\\btrue\\b'
+    'name': 'constant.language.boolean.less'
+


No blank newlines

winstliu · 2017-10-16T11:11:49Z

grammars/less.cson

+
+  'mixins':
+    'name': 'meta.mixin.less'
+    'begin': '(\\.[_a-zA-Z][a-zA-Z0-9_-]*)(?= *\\()'


What do you think about giving the . its own punctuation scope? Also, please use \\s* instead of <space>* for extra clarity.

Re: giving . it's own scope. Good idea. What would that be called?

Re: using \\s* vs *, I agree that's clearer, but the issue is that the space must be horizontal only. Is there a PCRE horizontal whitespace option? IIRC, I think that might be \\h?

Edit: Or is \\s only horizontal? Now I'm confusing myself. 🙈

You're correct that \\s includes vertical whitespace (like \n and \v), but in practice it doesn't really matter because matches are restricted to single lines only.

I would think that . should be scoped as punctuation.definition.mixin.less.

I see.. Good to know.

Re: punctuation.definition.mixin.less -- Sounds good to me.

winstliu · 2017-10-16T11:12:39Z

grammars/less.cson

+    'beginCaptures':
+      '0':
+        'name': 'meta.definition.mixin.less entity.name.mixin.less'
+    'end': '(?<=\\})'


{ and } don't need to be escaped.

winstliu · 2017-10-16T11:13:09Z

grammars/less.cson

+
+  'mixin_guards':
+    'name': 'meta.guard.less'
+    'begin': '(?:(when|and|when +not|and +not) *|(,) *)+(\\()'


Same here: use \\s.

winstliu · 2017-10-16T11:13:28Z

grammars/less.cson

+        'name': 'punctuation.separator.list.comma.css'
+      '3':
+        'name': 'meta.brace.round.css'
+    'end': '(\\))'


The outside capture can be removed, since we're only using the 0th capture.

winstliu · 2017-10-16T11:14:14Z

Can you please revert the spec structure to how it was originally?

dsifford · 2017-10-16T19:12:08Z

@50Wliu Welcome back!

Thanks for looking this over. I'll address these issues soon.

Re: crappy looking tests -- that was just temporary until you had time to get your eyes on it because I was being lazy 😝. I'll definitely fix that!

- still need to fix and add tests

dsifford · 2017-10-19T20:46:13Z

@50Wliu Should be ready for review now. Let me know if I missed something.

winstliu · 2017-10-19T21:27:35Z

spec/less-spec.coffee

+      expect(tokens[41]).toEqual value: '@', scopes: ['source.css.less', 'meta.mixin.less', 'meta.parameters.less', 'variable.parameter.less', 'punctuation.definition.variable.less']
+      expect(tokens[42]).toEqual value: 'c', scopes: ['source.css.less', 'meta.mixin.less', 'meta.parameters.less', 'variable.parameter.less']
+      expect(tokens[43]).toEqual value: ')', scopes: ['source.css.less', 'meta.mixin.less', 'meta.parameters.less', 'punctuation.definition.parameters.end.bracket.round.less']
+      expect(tokens[44].scopes).not.toContain 'meta.parameters.less'


Try to avoid the .not qualifier when writing language tests, because scopes can and do change often. Then if the new meta.parameters.less is added back this test will still pass. While more verbose, it's more useful to write out the exact scope list (i.e. just like tokens[43]).

Got it. Fixed.

winstliu · 2017-10-24T18:40:18Z

grammars/less.cson

+    'match': '\\btrue\\b'
+    'name': 'constant.language.boolean.less'
+  'variables':
+    'match': '(@|\\-\\-)[\\w-]+(?=\\s*)'


This lookahead seems rather useless since it can potentially match nothing.

Good catch. Not sure what I was thinking there...

winstliu · 2017-10-24T18:41:27Z

grammars/less.cson

+        'name': 'variable.parameter.less'
+      '1':
+        'name': 'punctuation.definition.variable.less'
+    'end': '(,)(?=\\s*@)'


The , doesn't need to have capture parentheses around it because the rest of the match is a lookahead. You can safely target the 0th capture.

Should be good to go now.

winstliu · 2017-10-28T15:22:13Z

@dsifford Sorry to bother you again, but here's another regression I'm seeing:

.markdown-preview[data-use-github-style] {
  .markdown-body(); // comment this out to fix everything

  padding: 30px; // padding is property-value
  font-size: 16px; // entity.name.tag.custom
  color: #333; // support.type.property-name.media
  background-color: #fff; // entity.name.tag.custom
  overflow: scroll; // no scope

  a {
    color: #337ab7;
  }
}

winstliu · 2017-11-01T11:27:27Z

Reverted for the time being.

dsifford · 2017-11-01T15:21:42Z

Sorry... this got buried in my notifications. I'll try to look at this this weekend

winstliu · 2017-11-01T19:00:52Z

Yeah no problem! I'm hoping that I'll have time to investigate as well.

begin work on improving mixin scopes

79282bb

add fixes suggested by @50Wliu

a0b97ca

dsifford commented Oct 4, 2017

View reviewed changes

dsifford added 2 commits October 4, 2017 20:29

finish initial mixin fix

808459f

all current tests passing

5774451

remove unnecessary comment

e2d01a5

dsifford mentioned this pull request Oct 6, 2017

Bash syntax highlighting bug when using comments in backticks microsoft/vscode#34643

Closed

This was referenced Oct 6, 2017

Add basic support for heredocs atom/language-puppet#51

Open

Fix window state not saving on close atom/atom#15514

Closed

winstliu reviewed Oct 16, 2017

View reviewed changes

winstliu added the under-review label Oct 16, 2017

dsifford added 5 commits October 16, 2017 20:44

remove blank lines

b73d8bc

add punctuation.definition.mixin.less scope to leading dot

617d7a0

fix all issues mentioned in PR

1dfe66c

- still need to fix and add tests

start adding tests

da3a865

finish tests

a48af08

winstliu reviewed Oct 19, 2017

View reviewed changes

remove negative assertion

83eb22c

winstliu reviewed Oct 24, 2017

View reviewed changes

address code review issues

ef4c3c3

winstliu merged commit 2802ab2 into atom:master Oct 25, 2017

winstliu mentioned this pull request Nov 1, 2017

Revert "Improve mixin scopes" #84

Merged

Conversation

dsifford commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work In Progress

Description of the Change

Todo

Uh oh!

winstliu commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsifford commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsifford Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winstliu commented Oct 16, 2017

Uh oh!

dsifford commented Oct 16, 2017

Uh oh!

dsifford commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winstliu commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winstliu commented Nov 1, 2017

Uh oh!

dsifford commented Nov 1, 2017

Uh oh!

winstliu commented Nov 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dsifford commented Oct 4, 2017 •

edited

Loading

dsifford Oct 16, 2017 •

edited

Loading

winstliu commented Oct 28, 2017 •

edited

Loading