Skip to content

Refactor VersionManager detection logic #3894

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.

Sign up for GitHub

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

Jump to bottom
Open
rafaelfranca wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Refactor VersionManager detection logic #3894

rafaelfranca wants to merge 14 commits into from

Conversation

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 8, 2026

Motivation

Looking at #3878 I noticed that adding a new version manager was requiring a lot of duplication of logic. While working on removing that I also noticed that even the existing manager were duplicating logic, so I took the opportunity to refactor all the managers.

Implementation

I moved the logic to detect the version manager to their classes. I also used a hash lookup instead of the exisint switch statement full of duplicated logic.

@rafaelfranca rafaelfranca requested a review from a team as a code owner January 8, 2026 03:53
@rafaelfranca rafaelfranca force-pushed the rm-refactor-version-manager branch from b480efc to 6e48de7 Compare January 8, 2026 03:55
@rafaelfranca rafaelfranca added chore Chore task vscode This pull request should be included in the VS Code extension's release notes labels Jan 8, 2026
@matheuscumpian matheuscumpian mentioned this pull request Jan 8, 2026
Merged
@rafaelfranca rafaelfranca force-pushed the rm-refactor-version-manager branch from 15de79c to 9d4ef87 Compare January 12, 2026 21:44
vinistock
vinistock reviewed Jan 22, 2026
View reviewed changes
vscode/src/ruby/rv.ts
// Learn more: https://github.com/spinel-coop/rv
export class Rv extends VersionManager {
private static getPossiblePaths(): vscode.Uri[] {
export class Rv extends Mise {
Copy link
Member

@vinistock vinistock Jan 22, 2026

Choose a reason for hiding this comment

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

This one I find a bit weird. I would instead extract another common parent rather than having two different version managers inherit from each other.

Copy link
Member Author

@rafaelfranca rafaelfranca Jan 22, 2026

Choose a reason for hiding this comment

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

I agree, but we have this same pattern with the Chruby and RubyInstaller, so I just simplified the hierarch chain of this one

@vinistock
Copy link
Member

vinistock commented Jan 22, 2026

Since this is a considerable refactor, we should cut a preview release of the extension once we converge on the implementation to catch any issues before the stable release.

@rafaelfranca rafaelfranca mentioned this pull request Jan 22, 2026
Merged
@rafaelfranca rafaelfranca changed the base branch from main to graphite-base/3894 January 22, 2026 21:15
@rafaelfranca rafaelfranca force-pushed the rm-refactor-version-manager branch from 06d1872 to 528f4a0 Compare January 22, 2026 21:15
@rafaelfranca rafaelfranca changed the base branch from graphite-base/3894 to rmf-vscode-shell January 22, 2026 21:15
@rafaelfranca Graphite App
Copy link
Member Author

rafaelfranca commented Jan 22, 2026 •
edited
Loading

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@rafaelfranca rafaelfranca force-pushed the rm-refactor-version-manager branch from 528f4a0 to f2567f4 Compare January 22, 2026 21:48
@rafaelfranca rafaelfranca mentioned this pull request Jan 22, 2026
Draft
@graphite-app graphite-app bot changed the base branch from rmf-vscode-shell to graphite-base/3894 January 26, 2026 16:46
graphite-app bot pushed a commit that referenced this pull request Jan 26, 2026
@rafaelfranca @graphite-app
Use configured shell from vscode.env.shell instead of process.env.SHE…
2f15b0b 
…LL (#3917)

This will make sure the detection system will use the configured shell in VS Code, and works without having on the SHELL environment variable set.

Extracted from #3894.
@graphite-app graphite-app bot force-pushed the rm-refactor-version-manager branch from f2567f4 to e1bf26b Compare January 26, 2026 17:17
@graphite-app graphite-app bot force-pushed the graphite-base/3894 branch from 845a130 to 2f15b0b Compare January 26, 2026 17:17
@graphite-app graphite-app bot changed the base branch from graphite-base/3894 to main January 26, 2026 17:18
@rafaelfranca
Make sure Auto doesn't behave as None and fail for developers
4485199 
This situation can only happen if discoverVersionManager() fails to
detect any version manager and leaves identifier as 'auto',
which should never happen since `None` is the fallback. This is just an
additional safety check to catch bugs early.
@graphite-app graphite-app bot force-pushed the rm-refactor-version-manager branch from e1bf26b to 4485199 Compare January 26, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paracycle paracycle paracycle approved these changes

@vinistock vinistock Awaiting requested review from vinistock

Assignees

@vinistock vinistock

Labels

chore Chore task vscode This pull request should be included in the VS Code extension's release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rafaelfranca @vinistock @paracycle