Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing scopes on separators and `self`, differentiate `->` #224

Open
wants to merge 12 commits into
base: master
from

Conversation

@chbk
Copy link

commented Dec 6, 2017

Description of the Change

  • Added a separators entry to the repository

  • Rewrote the grammar for class and module declarations with begin/end patterns:

    • Included separators
    • Renamed punctuation.separator.inheritance.ruby to punctuation.separator.namespace.ruby for consistency
    • Changed entity.name.type.class.ruby, entity.name.type.module.ruby and entity.other.inherited-class.module.ruby to start with an uppercase letter and to exclude separators

    This is now scoped properly:

    class A::B::C < ::D::E
    class << A::B
    module A::B::C
  • Scoped commas in function declarations:

    def method(a, b)
  • Scoped self and separators in function declarations:

    class A
        def self.method

    Unfortunately, I wasn't able to rewrite the grammar for function declarations with begin/end patterns. I used existing regular expressions to capture separators. Since the captures are placed inside repetition patterns ()*, only the last separator will be scoped. Not perfect, but still an improvement.

  • Changed scopes for ->: support.function.kernel.arrow.ruby

  • Added specs

Applicable Issues

#221 Missing scope on commas
#222 Differentiate -> from other keywords
#223 Namespace separator :: not scoped in class definition

@50Wliu 50Wliu added the needs-review label Dec 7, 2017
@Interdictor

This comment has been minimized.

Copy link

commented Mar 2, 2018

Please review and approve this!

@@ -60,60 +60,88 @@
'''
'patterns': [
{
'begin': '\\bclass\\b'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Do classes need to be at the start of the line per the previous ^\\s*class\\s* regex?

This comment has been minimized.

Copy link
@chbk

chbk Mar 7, 2018

Author

No, classes can be declared after another statement on the same line.

}
{
'begin': '<<'
'captures':

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

beginCaptures

@@ -60,60 +60,88 @@
'''
'patterns': [
{
'begin': '\\bclass\\b'
'captures':

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

beginCaptures

'include': '#separators'
}
{
'begin': '<<'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Seems like you could redo this as (<<)\\s* and directly add a contentName here to get rid of the inner pattern.

]
}
{
'begin': '<'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Same as concerns as above.

}
{
'begin': '\\bmodule\\b'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Same concerns as above.

}
{
'match': ';'
'name': 'punctuation.separator.statement.ruby'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Generally tokenized as punctuation.terminator.statement

}
{
'match': ','
'name': 'punctuation.separator.object.ruby'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

Generally tokenized as punctuation.separator.delimiter. Is , only for objects in Ruby?

This comment has been minimized.

Copy link
@chbk

chbk Mar 7, 2018

Author

There are also commas in hashes so punctuation.separator.delimiter is indeed better.

This comment has been minimized.

Copy link
@chbk

chbk Mar 7, 2018

Author

Is it okay if I replace all instances of punctuation.separator.object by the generic punctuation.separator.delimiter? Commas in method calls are tokenized by line 2325 but commas in method definitions have their own regexes.

'name': 'punctuation.separator.object.ruby'
}
{
'match': '(::)\\s*(?=[A-Z])'

This comment has been minimized.

Copy link
@50Wliu

50Wliu Mar 5, 2018

Member

The \\s* can be in the lookahead.

This comment has been minimized.

Copy link
@chbk

chbk Mar 7, 2018

Author

For some reason I thought lookaheads could only have a fixed number of characters.

grammars/ruby.cson Show resolved Hide resolved
@chbk

This comment has been minimized.

Copy link
Author

commented Mar 13, 2018

@50Wliu I have a commit pending for punctuation.separator.delimiter, is my suggestion okay to you?

@50Wliu

This comment has been minimized.

Copy link
Member

commented Mar 16, 2018

Yes, I think what you're saying is fine.

@chbk

This comment has been minimized.

Copy link
Author

commented Oct 17, 2018

@50Wliu Do you have any other comments on this pull request? It looks ready to merge for me.

@rsese

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Sorry for the delay @chbk, someone from the team will take a look as soon as they can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.