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

[Ruby] Improve keyword scopes #2156

Merged
merged 11 commits into from
Nov 21, 2019
Merged

Conversation

deathaxe
Copy link
Collaborator

This PR applies the #1228 and tries to find proper solutions for some special keywords/situations where needed.

DeathAxe added 5 commits October 27, 2019 14:16
This commit applies `storage.type.class` to the `class` type keyword.
This commit applies `storage.type.module` to the `module` type keyword.
The keyword `def` declares a method/function and is therefore scoped
as `storage.type.function`.
This commit applies sublimehq#1228.

Note:

The keywords `begin`, `end` and `do` are used in several situations to
open or close a block of code or entering a context. They are therefore
scoped as `keyword.control.block` as synonym for `meta.block` and
`punctuation.section.block.[begin|end]`.
This commit applies required changes caused by changes in Ruby.
Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
- match: '\b(class)\b'
scope: meta.class.ruby keyword.control.class.ruby
- match: \bclass\b(?=\s*<)
scope: storage.type.class.ruby
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm correct and this depicts a class method (idk Ruby), then shouldn't this be storage.modifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found examples the following example, which says something about to use that notation class << self just to avoid repetition of the class name in the method declaration.

Not sure whehter it should be scoped differently in such a situation.

class TestClass
  # bad
  def TestClass.some_method
    # body omitted
  end

  # good
  def self.some_other_method
    # body omitted
  end

  # Also possible and convenient when you
  # have to define many class methods.
  class << self
    def first_method
      # body omitted
    end

    def second_method_etc
      # body omitted
    end
  end
end

Copy link
Collaborator

@FichteFoll FichteFoll Oct 27, 2019

Choose a reason for hiding this comment

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

Okay, that is weird, to say the least. Looks more like a keyword.control.context than a class declaration, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. But I am unsure about using different scopes for the same keyword.

Copy link
Collaborator

@FichteFoll FichteFoll Oct 28, 2019

Choose a reason for hiding this comment

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

Using different scopes for the same keyword is no problem for the in operator (or keyword) in Python, for example. We prioritize semantics over syntax anyway and the alternative scope would still be a keyword.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure about that? in is imo a good example,
because it behaves entirely different.

for a in b:
    if 1 in a:
        pass

Unless you didn't mean to argue for the general case but about class in particular here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Off Topic: I saw several languages scoping in as keyword.operator. While keyword.operator.logical makes sense I am not sure about the use case in the for statement.

In class << self the class keyword has the same meaning as with or using. It would then need to be scoped as keyword.control.[block|context].

Sorry, but I still don't feel comfortable with keyword.control.[exception|resource] as discussed in #1228 because the former alternatives seem to be a more general approach, which can be applied to more situations/languages without sematical conflicts (see python or the keywords below).

Copy link
Contributor

@djspiewak djspiewak Nov 1, 2019

Choose a reason for hiding this comment

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

I generally am pretty happy to scope the same symbol differently in different contexts. A great example of this is => in Scala:

val f: Int => String = i => i.toString
//         ^^ keyword.operator.arrow
//                       ^^ storage.type.function.arrow.lambda

I think it's not at all unprecedented. (also, continuing my ongoing gripe with the names of scopes, the left arrow above is a type while the right arrow is a keyword indicating a lambda, but the scopes applied above are conventional and appropriate according to the guidelines)

Ruby is complicated and weird because it has things that are functions but behave like keywords, things that are keywords that behave like functions, and things that are neither that behave differently in different syntactic contexts. In general, I would say that the solution to the specific problems raised here (class and in) should be to scope them differently in the different contexts, biasing toward the differing semantics rather than toward the identical characters.

Copy link
Collaborator

@FichteFoll FichteFoll Nov 1, 2019

Choose a reason for hiding this comment

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

keyword.control.[exception|resource] has been replaced by keyword.control.context.[exception|resource] in my mind (or, ideally, keyword.context.[exception|resource]). I just haven't made a definitive statement on that yet because I was collecting input on a new second-level scope.

Edit: Ignore exception in this post. This was probably just a mixup on my part.

Copy link
Collaborator

@FichteFoll FichteFoll Nov 1, 2019

Choose a reason for hiding this comment

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

@djspiewak, you can add keyword.declaration to the second arrow, via #2156 (comment).

Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
Ruby/Ruby.sublime-syntax Outdated Show resolved Hide resolved
DeathAxe added 3 commits October 27, 2019 20:34
The keyword `alias` is somehow like `#define` in C. It is used to
declare an alias at compile time. The `undef` does the opposite.

As there are two directives for defining and undefining an alias, the
`storage.type` scope wouldn't be appropriate here.

An alternative would be `keyword.control.directive` which is used for 
preprocessor like keywords in Erlang.
DeathAxe added 3 commits November 7, 2019 20:27
In Ruby, `super` behaves like a magic function that resolves to the
appropriate function. Compared with keywords like `self` or `this` in
other languages, which are scoped as `variable.language`,
`support.function.builtin` seems to make most sense here and complies
with Python, even though the meaning is a little bit different here.
As super is treated as function, it should accept regexp like arguments.
@wbond
Copy link
Member

wbond commented Nov 11, 2019

So the general consensus is that this is good to merge?

@deathaxe
Copy link
Collaborator Author

deathaxe commented Nov 11, 2019

The two questions which we might need some feedback for are:

  1. What do you think about the keyword.control.block as a general scope for the keywords do, begin and end? We also discussed keyword.control.context or @FichteFoll even suggested something within a new 2nd level scope keyword.context. There are some comments about it at [RFC] Add new scopes to "jumping" flow control keywords/statements. #1228. The discussion started with [RFC] Add new scopes to "jumping" flow control keywords/statements. #1228 (comment) about keywords like using, with, ... which open a new block of code or manipulate the context for identifier lookups. It continued with this PR at [RFC] Add new scopes to "jumping" flow control keywords/statements. #1228 (comment) when it turned out we need something really general for that.

  2. The question about the scope for class << is still open. I currently decided to use storage.type.class keyword.declaration.class, but from the sematic aspect a scope from 1.) would be appropriate as well.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Nov 12, 2019

We might also want to consider (also in the context of #1228) whether die and panic (and exit in other languages) do the same and should be unified to a subscope of keyword.control.flow instead of using keyword.control.flow.<keyword-name>. But that's not as significant and can be talked about later.

@wbond
Copy link
Member

wbond commented Nov 21, 2019

@deathaxe I like keyword.control.block for 1. For 2 keyword.control.context seems most appropriate since I am familiar with the mechanics of Ruby. That said, I would also be okay with storage.type.class keyword.declaration.class.

@FichteFoll If I had to pick, I'd do something like keyword.control.flow.termination.

@wbond
Copy link
Member

wbond commented Nov 21, 2019

On second thought, I think storage.type.class keyword.declaration.class is much more correct. While class is being passed a block, it is really re-opening the definition and using the block to append the definition.

@deathaxe
Copy link
Collaborator Author

So everything would be ready to merge then, if keyword.control.flow.termination is addressed to die, panic, etc.

I've choosen keyword.control.flow.throw for throw, raise and fail here.

@wbond wbond merged commit 2710261 into sublimehq:master Nov 21, 2019
@wbond
Copy link
Member

wbond commented Nov 21, 2019

Thanks @deathaxe and everyone who spent time reviewing!

@deathaxe deathaxe deleted the pr/ruby/fix-keywords branch November 21, 2019 19:47
mitranim pushed a commit to mitranim/Packages that referenced this pull request Mar 25, 2022
* [Ruby] Fix 'class' scope name

This commit applies `storage.type.class` to the `class` type keyword.

* [Ruby] Fix 'module' scope name

This commit applies `storage.type.module` to the `module` type keyword.

* [Ruby] Fix 'def' scope name

The keyword `def` declares a method/function and is therefore scoped
as `storage.type.function`.

* [Ruby] Expand keywords scopes

This commit applies sublimehq#1228.

Note:

The keywords `begin`, `end` and `do` are used in several situations to
open or close a block of code or entering a context. They are therefore
scoped as `keyword.control.block` as synonym for `meta.block` and
`punctuation.section.block.[begin|end]`.

* [Rails] Fix test case

This commit applies required changes caused by changes in Ruby.

* [Ruby] Scope alias/undef as keyword

The keyword `alias` is somehow like `#define` in C. It is used to
declare an alias at compile time. The `undef` does the opposite.

As there are two directives for defining and undefining an alias, the
`storage.type` scope wouldn't be appropriate here.

An alternative would be `keyword.control.directive` which is used for 
preprocessor like keywords in Erlang.

* [Ruby] tweak declaration keyword scopes

* [Ruby] Merge fail,raise,throw scopes

* [Ruby] Scope super as function

In Ruby, `super` behaves like a magic function that resolves to the
appropriate function. Compared with keywords like `self` or `this` in
other languages, which are scoped as `variable.language`,
`support.function.builtin` seems to make most sense here and complies
with Python, even though the meaning is a little bit different here.

* [Ruby] Allow regexp like super arguments

As super is treated as function, it should accept regexp like arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants