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

Converted chruby to generic ruby #437

Merged
merged 12 commits into from
Sep 14, 2023
Merged

Conversation

cmer
Copy link
Contributor

@cmer cmer commented Sep 3, 2023

Description

Another Ruby implementation, following in the footsteps of #240.

I converted the chruby implementation to something that is more generic and supports any Ruby version manager such as chruby, rvm, asdf or even system installations.

CleanShot 2023-09-03 at 11 17 20@2x

How Has This Been Tested

  • I have tested using Linux.
  • I have tested using MacOS.

Checklist

  • I am ready to update the wiki accordingly.
  • I have updated the tests accordingly.

@cmer cmer requested a review from IlanCosman as a code owner September 3, 2023 14:45
@cmer cmer mentioned this pull request Sep 3, 2023
4 tasks
@IlanCosman
Copy link
Owner

Thanks for working on this! I refactored the code to be faster and smaller. It should hopefully have the same behavior 🙂

A few thoughts:

1

I don't love searching every directory above us for anything ending in .gemspec. It might get unreasonable on pathological filesystems, and anything that can happen will happen 😄

I have absolutely no ruby-world knowledge, but according to my very brief research (https://piotrmurach.com/articles/writing-a-ruby-gem-specification/), the convention is to name the file package.gemspec. Do you think just searching for that would work?

2

Do we want to include .tool-versions in this search list? Is there any guarantee that the user is actually using the .tool-versions file to specify a ruby version, or could they be using it for something else and we're just showing useless information? This is the disadvantage of a general version manager I suppose.

@halostatue
Copy link
Contributor

I have absolutely no ruby-world knowledge, but according to my very brief research (https://piotrmurach.com/articles/writing-a-ruby-gem-specification/), the convention is to name the file package.gemspec. Do you think just searching for that would work?

It would not work. The convention is package.gemspec. That is, I maintain a package called diff-lcs. Its gemspec is diff-lcs.gemspec.

2

Do we want to include .tool-versions in this search list? Is there any guarantee that the user is actually using the .tool-versions file to specify a ruby version, or could they be using it for something else and we're just showing useless information? This is the disadvantage of a general version manager I suppose.

We would not, IMO. If it .tool-versions is found, it would be better to inspect it to see if it contains ^ruby\s+\d. Otherwise, ruby --version might error out.

functions/_tide_item_ruby.fish Outdated Show resolved Hide resolved
@cmer
Copy link
Contributor Author

cmer commented Sep 11, 2023

Thanks for the feedback, @IlanCosman . To answer your questions:

We actually do need to search for *.gemspec because gems typically don't use the name package.gemspec. For example, look at this gem that is part of Rails: https://github.com/rails/rails/tree/main/actioncable

The reason I was looking for .tool-versions was because I wanted to know which Ruby version is active in any directory I am in. But I understand others might not want that, so it's totally reasonable to remove it.

@cmer
Copy link
Contributor Author

cmer commented Sep 11, 2023

Would it be overkill to also look for *.rb?

@halostatue
Copy link
Contributor

The reason I was looking for .tool-versions was because I wanted to know which Ruby version is active in any directory I am in. But I understand others might not want that, so it's totally reasonable to remove it.

I can understand this, but I think that this is a viable request for pretty much any of the widgets. Something like _tide_always ruby node and checking for contains -- ruby $_tide_always in the Ruby plug-in would be a better approach for this sort of thing. Something like $_tide_always would be a different feature/pull request, though.

With respect to *.rb…I do think it would be overkill to search all parents; I would maybe search the current directory for .rb files, but even that is probably overkill.

@cmer
Copy link
Contributor Author

cmer commented Sep 11, 2023

Cool let's merge this bad boy!

Copy link
Owner

@IlanCosman IlanCosman left a comment

Choose a reason for hiding this comment

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

Seems good to go, thanks for contributing :)

@IlanCosman IlanCosman merged commit 9252158 into IlanCosman:main Sep 14, 2023
5 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants