Tool specs and fetch/install/pin refactoring - #234
Merged
Conversation
mikrostew
changed the title
charlespierce
approved these changes
Jan 10, 2019
charlespierce
left a comment
charlespierce
left a comment
Contributor
There was a problem hiding this comment.
Looks great! So much tool-specific code deleted!
dherman
requested changes
Jan 15, 2019
dherman
left a comment
dherman
left a comment
Collaborator
There was a problem hiding this comment.
Awesome work! I have a few questions about some of the type structure. It might help to chat about some of them -- ping me on discord whenever.
Collaborator
|
@mikrostew and I spent some time thinking about how to make it more static and it was nonobvious enough that I think it's better to go with the enum for now and file a code quality issue for making it more static later. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
I was planning to only do #173, but some of that was also tangled up with #174, so I combined both into this PR.
Notes
First thing I implemented was
tool::ToolSpec, to encapsulate a tool (Node/Yarn/Npm/Npx/Package) and aVersionSpec. This makes it easy to use for fetch/install/pin, which I abstracted to general-purpose functions instead of having things likeinstall_nodeandinstall_yarnandinstall_xxx. That in turn made the command implementation simpler, since they no longer have to parse out the tool and figure out which function to call.I also initially implemented a
tool::Toolenum (like in #173), but didn't need it for anything so I took it back out.I didn't move the
shimcode intotool::ToolSpecas mentioned in #173. It didn't seem to fit at this point, but that may need to be part of #175.Bonus
Fixed a bug in
notion-core/src/toolchain/mod.rswhere the platform file had been changed to using JSON (in #220), but saving an empty platform instance would result in[platform]being written to the file.Closes #173
Closes #174
Closes #87