georgewfraser / vscode-tree-sitter

Accurate syntax coloring for VSCode using tree-sitter
MIT License
175 stars 25 forks source link

Ruby improvements #16

Closed connorshea closed 5 years ago

connorshea commented 5 years ago

This resolves some issues I pointed out in #12.

Ruby method parameters

Before

Screen Shot 2019-05-26 at 4 46 09 PM

After

Screen Shot 2019-05-26 at 4 45 49 PM

Ruby keywords

This doesn't quite catch everything because the method catchers are a bit too aggressive (e.g. attr_reader :variable_name should have attr_reader caught as a keyword but is not), will need to fix that later.

Before

Screen Shot 2019-05-26 at 4 46 50 PM

After

Screen Shot 2019-05-26 at 4 47 03 PM

Changed symbols from constant.numeric to constant.language

Before

Screen Shot 2019-05-26 at 4 49 01 PM

After

Screen Shot 2019-05-26 at 4 49 08 PM
connorshea commented 5 years ago

I also replaced some non-null assertion operators since they were causing errors when they were actually able to be null.

georgewfraser commented 5 years ago

You've added a bunch of keywords like attr_reader that aren't actually keywords according to the Ruby language spec:

https://docs.ruby-lang.org/en/2.2.0/keywords_rdoc.html

The tree-sitter grammar doesn't recognize them as keywords either, try putting

class Foo
    protected

    def foo
        1
    end

    private

    def bar 
        1
    end
end

into http://tree-sitter.github.io/tree-sitter/playground and you get

program [0, 0] - [13, 0])
  class [0, 0] - [12, 3])
    constant [0, 6] - [0, 9])
    identifier [1, 1] - [1, 10])
    method [3, 4] - [5, 7])
      identifier [3, 8] - [3, 11])
      integer [4, 5] - [4, 6])
    identifier [7, 4] - [7, 11])
    method [9, 4] - [11, 7])
      identifier [9, 8] - [9, 11])
      integer [10, 5] - [10, 6])

Can you help me understand how the tokens private and protected relate to the class here? Can I put any identifier in these positions?

georgewfraser commented 5 years ago

I merged this but I undid the method parameter highlighting because it makes them inconsistent with references to those parameters: https://github.com/georgewfraser/vscode-tree-sitter/commit/c99765fa45c0966b0c92bc18bcb38ec84e707cc1

Also I would still like your feedback on the above question, this part feels like a hack that should be done by tree-sitter:

https://github.com/georgewfraser/vscode-tree-sitter/blob/c99765fa45c0966b0c92bc18bcb38ec84e707cc1/textmate/ruby.tmLanguage.json#L20

connorshea commented 5 years ago

Oh you're correct, interesting. Most of those are actually methods of Module, though not all Module methods are represented by that list.

I've always seen protected, private, etc. as being keywords because they were colored as such by most highlighters (GitHub's highlighter does it, for example).

In this commit, private/protected/public are made to be keywords in the tree-sitter: https://github.com/tree-sitter/tree-sitter-ruby/commit/82c0e2a78274fe16df6bc7f8a4d4af54995ad6c7

Weirdly, it looks like they're only considered keywords by the CSS? I'm not really sure why it's done that way.

public, private, and protected are typically used without an argument, and restrict all subsequent methods in the class/module to that type.

You can also use them with an argument, like private :method_name, to make a specific method or list of methods private.

class Example
  def methodA
  end

  private # all following methods in this class are private, and are inaccessible to outside objects

  def methodP
  end
end
class Example
  def methodA
  end

  def methodP
  end

  private :methodP
end

attr_accessor, attr_writer, and attr_reader are also colored as keywords by most highlighters, but they're not handled in any special way by the tree-sitter as far as I can tell. There's less of an argument to be made that these should be considered keywords.

As for the method parameter colorization being inconsistent, I'd disagree with that being a problem but I can definitely see your point. My argument there would be that "it's how everyone else does it", but that's not a very good argument :)

Unfortunately it'd be quite difficult to consistently colorize those variables since tree-sitter doesn't distinguish between method parameters and any other variables when used in the method itself.

connorshea commented 5 years ago

Oh and you can add a random variable in the middle of the class, but it wouldn't actually do anything as far as I'm aware. public/private/protected are special in that regard.

class Example
  def methodA
  end

  foo

  def methodP
  end
end
connorshea commented 5 years ago

So I discussed this with some other Rubyists because I couldn't really figure out why I felt like these should be highlighted as keywords even though they aren't. e.g. why should require be highlighted as a keyword when puts shouldn't, despite both being methods of Kernel.

The two responses I think fit best with my opinion are:

These were specifically in regards to require, require_relative, include, private, protected, public, raise, and attr_reader/attr_writer/attr_accessor.

I'm not sure if this necessary fits with the philosophy of the extension, that'd be your call :) Admittedly, it's pretty arbitrary and just based on what existing highlighters do.

georgewfraser commented 5 years ago

It sounds like they should be keywords, if they are nested under a class. So private should be a keyword here:

class Foo
  private
end

and here:

class Foo
  private :foo
end

but not here:

private = 1