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 improvements #16

Merged
merged 4 commits into from
May 27, 2019
Merged

Conversation

connorshea
Copy link
Contributor

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

src/extension.ts Outdated Show resolved Hide resolved
Also replaced some non-null assertion operators to prevent failures when they are, in fact, null.
@connorshea
Copy link
Contributor Author

connorshea commented May 26, 2019

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

@georgewfraser
Copy link
Owner

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 georgewfraser merged commit 5bdb252 into georgewfraser:master May 27, 2019
@georgewfraser
Copy link
Owner

I merged this but I undid the method parameter highlighting because it makes them inconsistent with references to those parameters: c99765f

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

"match": "\\b(initialize|new|loop|include|extend|prepend|raise|fail|attr_reader|attr_writer|attr_accessor|attr|catch|throw|private|private_class_method|module_function|public|public_class_method|protected|refine|using)\\b(?![?!])",

@connorshea
Copy link
Contributor Author

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: tree-sitter/tree-sitter-ruby@82c0e2a

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
Copy link
Contributor Author

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
Copy link
Contributor Author

connorshea commented May 27, 2019

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:

  • "I consider those to be keywords in the context of how they relate to other programming languages where they would be a keyword"
  • "they're not keywords but they have important (critical) semantics"

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
Copy link
Owner

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

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.

None yet

2 participants