-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor away def_file
module into CommandVersionRegistry
#275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is going to be a replacement for `def_file`. This type manages the entire state of the definitions.
This first integration is not perfect but it replaces the usage of def_file which is a good start.
This was done while grouping a lot of their common logic into a single code path. This has significantly simplifies the code.
This also adds a `CommandVersion` struct which ends up making the registry easier to work with.
def_file
module into CommandVersionRegistry
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is in preparation for fixing #274
At its core, this PR removes the
def_file
module in favor of a newcommand_version::CommandVersionRegistry
type which manages the command version known byalt
. This replacement works with the same file as before:$ALT_HOME/defs.toml
. This new type wraps all operations around command versions and regroups all operations into a single module.This refactor pulls all the various bits of code that operate on the old
defs
type into theCommandVersionRegistry
type. This simplifies a lot of the code of the various commands who no longer have to fiddle with the internals of thedefs.toml
dictionary.This comes with a few other refactors:
anyhow
crate. This is nicer than the old pattern of just calling.expect(...)
or.unwrap(...)
. This should make the errors reported byalt
a little clearer and they should include more context.config
module toenvironment
. Previously, theconfig
module would only deal in paths likealt
's home directory. The new module is now also able to load theCommandVersionRegistry
from$ALT_HOME/defs.toml
file. Whileenvironment
is about as generic asconfig
, matches more with the idea of "the environmentalt
runs in" which is what this module represents.CommandVersion
from thescan
module with the newCommandVersion
type that's part of the newcommand_version
module.use_cmd
module, replace the"system"
magic string with an enum type that represents the possibility between using the system version of a command or to use a specific one.The refactor here brings a new set of patterns for the code. It doesn't apply this to the whole codebase and focuses on what needs to change for #274 . For example, the error management with
anyhow
is how I want all the code to look but I didn't refactor it all.