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

Fallback to latest known Ruby if no .ruby-version is found #2836

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Nov 6, 2024

Motivation

Completes a significant part of handling missing .ruby-version for #2793

If the user doesn't have a .ruby-version in their project or in any parent directories, then we can try to fallback to the latest known Ruby available.

Note: this PR doesn't handle the aspect of having a .ruby-version configured to a Ruby that's not installed. I'll follow up with that later.

Implementation

To avoid having this behaviour be surprising, I used a progress notification with a 5 second delay warning the user that we are going to try falling back to the latest known Ruby. If they don't do anything, we search for the Ruby installation and launch.

If the user clicks cancel, then we stop everything and offer them 3 options:

  1. Create a .ruby-version file in a parent directory. Here we use a quick pick to list all known rubies and create the file for them using what they select
  2. Manually configure a global fallback Ruby installation for the LSP
  3. Shutdown

Automated Tests

Added automated tests for two scenarios. I haven't figured out if it's possible to trigger the cancellation in a test even with a stub, so I failed to create tests for those cases.

If you have an idea about how to fake the cancellation of the progress notification, please let me know!

Copy link
Member Author

vinistock commented Nov 6, 2024

@vinistock vinistock added other Changes that aren't bugfixes, enhancements or breaking changes vscode This pull request should be included in the VS Code extension's release notes labels Nov 6, 2024 — with Graphite App
@vinistock vinistock marked this pull request as ready for review November 6, 2024 19:03
@vinistock vinistock requested a review from a team as a code owner November 6, 2024 19:03
@vinistock vinistock force-pushed the 11-06-allow_individual_version_managers_to_trigger_manual_ruby_selection branch from b9e537c to ba965f8 Compare November 6, 2024 19:04
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from ea45f37 to 4bf4561 Compare November 6, 2024 19:04
@vinistock vinistock force-pushed the 11-06-allow_individual_version_managers_to_trigger_manual_ruby_selection branch from ba965f8 to 04bc155 Compare November 19, 2024 18:18
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from 4bf4561 to 9a4bd56 Compare November 19, 2024 18:18
@vinistock vinistock force-pushed the 11-06-allow_individual_version_managers_to_trigger_manual_ruby_selection branch from 04bc155 to e0025a6 Compare November 19, 2024 18:34
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from 9a4bd56 to b035193 Compare November 19, 2024 18:34
vscode/src/ruby/chruby.ts Outdated Show resolved Hide resolved
@vinistock vinistock changed the base branch from 11-06-allow_individual_version_managers_to_trigger_manual_ruby_selection to graphite-base/2836 November 20, 2024 18:33
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from b035193 to edf4f00 Compare November 20, 2024 18:33
@andyw8
Copy link
Contributor

andyw8 commented Nov 20, 2024

As discussed:

  • Increase the time we show the initial dialog (to 10s or 15s)
  • Since the labels are being truncated, let's add a description above so that we can use more concise labels.
  • Let's allow the user to choose which folder .ruby-version is written to, rather the parent, to avoid potential problems running other Ruby projects on their system.
  • Change configure messsage to "Configure global fallback or workspace specific Ruby for Ruby LSP?"
  • Remove 'shutdown' button (can use X instead)
  • Change "The Ruby LSP requires a Ruby version to launch. You can define a fallback by adding a .ruby-version to a parent directory or by configuring a fallback" to make it clear that one setting is for your system, the other is only for the Ruby LSP extension, e.g.:

"The Ruby LSP requires a Ruby version to launch. You can define a system or Ruby LSP fallback to use"

Buttons: [System fallback] [Ruby LSP fallback]
(adjust placeholders/title as necessary)

@vinistock vinistock changed the base branch from graphite-base/2836 to main November 20, 2024 18:34
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch 3 times, most recently from eecb394 to 21ecd46 Compare November 20, 2024 19:57
@vinistock
Copy link
Member Author

I addressed all of the feedback. Ready for another round of reviews.

@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from 21ecd46 to f8abd81 Compare November 22, 2024 18:36
vscode/src/ruby/chruby.ts Outdated Show resolved Hide resolved
vscode/src/ruby/chruby.ts Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch from f8abd81 to 25612c3 Compare November 22, 2024 18:46
@vinistock vinistock merged commit 6e5501d into main Nov 22, 2024
38 checks passed
Copy link
Member Author

Merge activity

  • Nov 22, 2:12 PM EST: A user merged this pull request with Graphite.

@vinistock vinistock deleted the 11-06-fallback_to_latest_known_ruby_if_no_.ruby-version_is_found branch November 22, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
other Changes that aren't bugfixes, enhancements or breaking changes vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants