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

[#57] Adds install command to install known tools #89

Merged
merged 5 commits into from
Sep 15, 2022

Conversation

MitchellBerend
Copy link
Collaborator

@MitchellBerend MitchellBerend commented Sep 15, 2022

Resolves #57

This pr adds an isntall subcommand that lets the user install any of the hardcoded tools. As a bonus it also implements the From trait to convert from ToolInfo To ConfigAsset.

Additional tasks

  • Documentation for changes provided/changed
  • Tests added

@MitchellBerend MitchellBerend marked this pull request as ready for review September 15, 2022 14:55
@MitchellBerend
Copy link
Collaborator Author

Im not sure what tests can be added to make sure everything works properly.

Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Really nice! I'm surprised how easy you implemented this command! 🤯

I have a few suggestions to improve the code and some ideas for future improvements but this MVP is already good enough to be shipped with the v0.2.0 release after a few code changes 👍🏻

README.md Show resolved Hide resolved
src/config/cli.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 39 to 48
Command::Install { name } => {
if let Some(tool_info) = lookup_tool(&name) {
if let Ok(mut tool) = toml::parse_file(&config_path) {
let tool_btree: BTreeMap<String, ConfigAsset> =
BTreeMap::from([(name, tool_info.into())]);
tool.tools = tool_btree;
sync(tool);
}
}
}
Copy link
Owner

@chshersh chshersh Sep 15, 2022

Choose a reason for hiding this comment

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

Ideas for future improvements (can be done as separate issues):

  • Update the config to add a new tool (so it later will be updated with sync)
  • Support all ConfigAsset fields as arguments to the install commandj
  • Allow downloading the tool from the parsed config if it's already specified in there

@chshersh
Copy link
Owner

Im not sure what tests can be added to make sure everything works properly.

I think we can add similar integration tests in the ci.yml by calling the tool install command and verifying the tool exists. I have an idea to simplify the testing setup but writing a few lines of YAML should be okay for now 👍🏻

Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

That looks great 👏🏻

src/config/cli.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@chshersh chshersh added enhancement New feature or request CLI Command Line Interface labels Sep 15, 2022
@chshersh chshersh merged commit 2bb99cf into chshersh:main Sep 15, 2022
@MitchellBerend MitchellBerend deleted the 57-install-command branch September 17, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Command Line Interface enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the 'tool install' command
2 participants