Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix a bug in the grammar of @supports #164

Merged
merged 3 commits into from
Aug 10, 2016
Merged

Fix a bug in the grammar of @supports #164

merged 3 commits into from
Aug 10, 2016

Conversation

hediyi
Copy link
Contributor

@hediyi hediyi commented Aug 9, 2016

in the commit message of 698e3c4

'logical_operators':
'match': '\\b(==|!=|<=|>=|<|>|not|or|and)\\b'
'name': 'keyword.control.operator'
'match': '\\b(?:not\\b|or\\b|and\\b)'
Copy link
Contributor

Choose a reason for hiding this comment

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

\\b(not|or|and)\\b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is \\b(not\\b|or\\b|and\\b) ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performance. 😆

@winstliu
Copy link
Contributor

winstliu commented Aug 9, 2016

Please rebase to get rid of the ellipse commit.

Problems:
* `\\b==` does not allow spaces before ==
* 'keyword.control.*' is for keywords like `while` or `@supports` by
  convention, but not these operators
* operators like == do not fit in the name 'logical_operators'
The grammar of @supports at the moment cannot distinguish between
properties and values: for a word or word-hyphen combination that is
being used as both property and value, "flex" in this case, it will
always be tokenized as a value.
@hediyi
Copy link
Contributor Author

hediyi commented Aug 10, 2016

Anything else?

@winstliu
Copy link
Contributor

Just to increase my understanding of things: how does this fix the referenced issue? Is it because #properties puts things in the correct order, whereas the current manual order did it wrong?

@hediyi
Copy link
Contributor Author

hediyi commented Aug 10, 2016

First, the problem is actually I overlooked the fact that some word can be both a property and a value in CSS—flex in this case (can't say there won't be more in the future)—in the old way, it will always be tokenized as either a property or value, depending on, #property_names and #property_values, which comes first in the grammar.

{
  'include': '#constant_property_value'
}
{
  'include': '#property_names'
}
{
  'include': '#property_values'
}

In the old way, at first, there were just #property_names and #property_values (flex always tokenized as property name), I patched it by placing a #constant_property_value at the beginning (now always value) to make the test pass. But as you can see in the new specs, this didn't really solve the problem.

The fix mainly comes from that #properties wraps a layer around #property_values which specifies when exactly a token can be a value (begin and end):

{
  'begin': '(:)\\s*(?!(\\s*{))''end': '\\s*(;|(?=}|\\)))''contentName': 'meta.property-value.scss'
  'patterns': [
    …
    {
      'include': '#property_values'
    }
  ]
}

@winstliu
Copy link
Contributor

Ok, that's what I was thinking, but just wanted to double-check. Thanks.

@winstliu winstliu closed this Aug 10, 2016
@winstliu winstliu reopened this Aug 10, 2016
@winstliu winstliu merged commit 5396608 into atom:master Aug 10, 2016
@winstliu
Copy link
Contributor

(Wrong button 🙈)

@hediyi hediyi deleted the fix-at-supports branch August 10, 2016 12:01
@hediyi
Copy link
Contributor Author

hediyi commented Aug 10, 2016

No problem 😆

@hediyi hediyi restored the fix-at-supports branch August 10, 2016 12:05
@hediyi hediyi deleted the fix-at-supports branch August 10, 2016 12:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants