Refactor VersionManager detection logic #3894
Conversation
rafaelfranca
force-pushed
the
rm-refactor-version-manager
branch
from
b480efc to
6e48de7
Compare
rafaelfranca
added
chore
rafaelfranca
force-pushed
the
rm-refactor-version-manager
branch
from
15de79c to
9d4ef87
Compare
| // Learn more: https://github.com/spinel-coop/rv | ||
| export class Rv extends VersionManager { | ||
| private static getPossiblePaths(): vscode.Uri[] { | ||
| export class Rv extends Mise { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree, but we have this same pattern with the Chruby and RubyInstaller, so I just simplified the hierarch chain of this one
|
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. |
…ss as a static method
rafaelfranca
force-pushed
the
rm-refactor-version-manager
branch
from
06d1872 to
528f4a0
Compare
rafaelfranca
changed the base branch from
graphite-base/3894
to
rmf-vscode-shell
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd 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. |
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.
rafaelfranca
force-pushed
the
rm-refactor-version-manager
branch
from
528f4a0 to
f2567f4
Compare
3 participants
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
switchstatement full of duplicated logic.