Skip to content

Add initial support for Ruby language #134

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

Closed
wants to merge 1 commit into from

Conversation

mcritchlow
Copy link

I'm still very new to all of this, but this is started to work for me with some local testing.

Any feedback or comments on whether this is anywhere close to mergeable would be great!

This is my first go at any Lua, so if there are better idiomatic ways of doing some of this I'm all ears :)

@mcritchlow mcritchlow marked this pull request as ready for review November 16, 2021 21:09
@ThePrimeagen
Copy link
Owner

ThePrimeagen commented Nov 17, 2021

Congrats on your first pr!!!!

So it is hard for me to "accept" a new language considering I have not quite finished the extract functionality to a set that is useful. Upgrading languages is non trivial. Are you stating here that you are willing to support the progression of ruby?

Also, run make pr-ready to warn you on lint / run tests / enforce style standards.

@mcritchlow
Copy link
Author

Sorry for the delayed response. Thanks!

So it is hard for me to "accept" a new language considering I have not quite finished the extract functionality to a set that is useful. Upgrading languages is non trivial. Are you stating here that you are willing to support the progression of ruby?

That's totally fair, and I'm not completely sure how to answer the question. I'm motivated to have this functionality for Ruby, because it's what I work with daily and I really like the way you're laying out this project. Having said that, I'm not sure what your plans are for things right now and how much churn there is likely to be. I'd hate for me to not be able to keep the pace up with your changes and have the Ruby language support become a bottleneck. Would it make more sense for things to stabilize a bit more from your perspective prior to adding additional languages? If so, I can always try and keep my fork more or less up to date and then reconsider submitting this again when it's more appropriate. Long way of saying, I could just say "yes!" but I want to be realistic also :)

Also, run make pr-ready to warn you on lint / run tests / enforce style standards.

Will do, thanks. I'll get this integrated so that regardless of the decision with this PR for now I'm in a better place to enforce all the standards.

Thanks again.

@olimorris
Copy link
Contributor

Just chiming in as ruby support for the plugin would be amazing.

@ThePrimeagen what's your view on accepting PRs for other languages now? I appreciate what incorporating into the plugin means from a support perspective and the freedom with which you can change the API etc. Alternatively, do you ever envisage a feature that would allow third-parties to "hook" into refactoring.nvim and we have a ruby source which we could pull into the plugin? Or, would you ever consider having "Supported Languages" and "Third Party Supported Languages" with the disclaimer that you won't support the latter?

@otavioschwanck
Copy link

Ruby support, YAY <3

@otavioschwanck
Copy link

image

This error is happening...

@mcritchlow
Copy link
Author

@otavioschwanck yeah, my original branch is now not in line with the current state of this plugin. I wouldn't expect it to work as-is at the moment. This PR was an initial pass that minimally worked with how the extract functionality worked at the time, about 4 months ago.

My original question still stands in this regard.

Would it make more sense for things to stabilize a bit more from your perspective prior to adding additional languages?

I suspect the answer is yes :)

@ThePrimeagen
Copy link
Owner

@mcritchlow we are in our final version and likely no big changes planned.

So it makes sense at this point, if you are willing, to support Ruby.

@mcritchlow
Copy link
Author

@ThePrimeagen - oh neat, thanks! Ok I'll try and carve out some time next week to rebase and see what's needed to update. I remember having some questions/concerns about the treesitter queries but if that comes up again I'll maybe ask you here or elsewhere about that.

@TortoiseHive
Copy link

@ThePrimeagen - oh neat, thanks! Ok I'll try and carve out some time next week to rebase and see what's needed to update. I remember having some questions/concerns about the treesitter queries but if that comes up again I'll maybe ask you here or elsewhere about that.

Join the Primeagen discord, it have a section only for the refactoring plugin

@olimorris
Copy link
Contributor

@mcritchlow happy to test it out next week 👍🏼

@mcritchlow
Copy link
Author

I wanted to follow up here since I'd hoped to get back to this a week or so ago and I know a few folks here are interested. This is still on my list, but there are a number of other things going on right now that are more of a priority for me in my free time.

Which is just to say, I do plan to get back to this as it is something I'm interested in. But, if there's anyone more immediately interested and motivated to move this forward, please do.

@olimorris
Copy link
Contributor

@mcritchlow - I've started work on this in a seperate fork here. You can see where it's failing with make test. I need to add Treesitter for Ruby next.

Would love to collaborate with you on this.

@mcritchlow
Copy link
Author

@olimorris - that's awesome! I'll try and take a look this weekend or early next week. Thanks for continuing with this :)

@olimorris
Copy link
Contributor

@olimorris - that's awesome! I'll try and take a look this weekend or early next week. Thanks for continuing with this :)

I'll start opening up some issues on my repo so we can discuss over there.

@olimorris
Copy link
Contributor

I've just created a pull request with support for 106 and 119.

@pranavrao145
Copy link
Collaborator

Since #326 has been merged, I will close this issue now. Thanks everyone for your contributions!

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.

6 participants