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 TextObjects and more file extensions #2143

Merged

Conversation

danillos
Copy link
Contributor

@danillos danillos commented Apr 17, 2022

It includes Ruby TextObjects and more Ruby file extensions

It is my first time working with Tree-Sitter, improvements are welcome.

@danillos danillos changed the title Ruby TextObjects and File Extensions Ruby TextObjects and more file extensions Apr 17, 2022
@danillos danillos marked this pull request as ready for review April 17, 2022 13:27
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This is a bit of a nit but lines 3 and 24 are indented by one space. Can we use two for the sake of consistency?

@danillos danillos marked this pull request as draft April 17, 2022 23:01
@the-mikedavis
Copy link
Member

What do you think about a class selector for singleton classes? (I'm not sure if I have the right terminology here.)

https://github.com/appsignal/appsignal-ruby/blob/f97cc5a7b3c8713929faf705cdf2266a7583c8d4/lib/appsignal.rb#L18

module Foo
  class << self
    # ..
  end
end

I think a query like

(singleton_class
  value: (_)
  (_)+ @class.inside) @class.around

might cover it.

@danillos
Copy link
Contributor Author

What do you think about a class selector for singleton classes? (I'm not sure if I have the right terminology here.)

appsignal/appsignal-ruby@f97cc5a/lib/appsignal.rb#L18

module Foo
  class << self
    # ..
  end
end

I think a query like

(singleton_class
  value: (_)
  (_)+ @class.inside) @class.around

might cover it.

Nice, I added it too.

@danillos
Copy link
Contributor Author

danillos commented Apr 18, 2022

I did tests for Classes using these variations:

class Bar
  # ...
end

class Bar < Foo
  # ...
end

class Bar < Foo::Test
  # ...
end

class Bar::Foo
  # ...
end

class Bar::Foo < Foo
  # ...
end

class Bar::Foo < Foo::Test
  # ...
end

module Bar
  class Foo
    # ...
  end
end

module Bar
  class Foo < Bar
    # ...
  end
end

class Bar
  class Foo
    # ...
  end
  
  def test
    # ...
  end
end

module Foo
  class << self
    # ...
  end
end

Class.new do
  # ...
end

But I had to do this test manually. @the-mikedavis do you know if we have Unit Tests for it? I'm pretty sure that a future update on https://github.com/tree-sitter/tree-sitter-ruby could break it.

@danillos danillos marked this pull request as ready for review April 18, 2022 00:21
@the-mikedavis
Copy link
Member

There isn't currently test infrastructure for queries although I think it's a good idea.

Highlights queries could use the existing tree-sitter testing syntax and textobjects might be able to use some recent testing utilities around selections within Helix.

We pin the grammar repository versions in languages.toml so if the repo updates it won't break queries here until we update the version. Updating the grammars (manually) can be a bit error prone (e.g. #2153) but generally Helix fails loudly if the queries are invalid so it's somewhat easy to detect. But a testing framework would make that a lot easier to be confident about.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, these look great!

@the-mikedavis the-mikedavis merged commit be656c1 into helix-editor:master Apr 18, 2022
@stuarth
Copy link
Contributor

stuarth commented Apr 18, 2022

thanks for this @danillos!

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.

3 participants