-
Notifications
You must be signed in to change notification settings - Fork 143
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
feat: support toolchain export #526
base: master
Are you sure you want to change the base?
Conversation
@sdankel GM sir, looking for your review again😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! 🚀
…o feat/export-toolchain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests! I left a few more comments, and will test this out next week.
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, left some nits.
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running toolchain export
without any args / options I see the following
I would move the message asking for input onto a new line instead of having a single long line.
The default is not automatically exported, it prompts for input.
The invalid channel
message comes from a break in backwards compatibility. I haven't followed the changes to know how we manage new toolchains etc. so I'm unsure if this x86
channel is valid but I have updated fuelup and installed beta-4
so I assume that it's still valid.
All that to say, we are forcing a new format, have we considered the current / old format and whether we ought to support that?
Running toolchain export
with beta-4
I get a file containing the toolchain and components however when I run latest-YYYY-MM-DD
I get the following error
We ought to handle the difference between a valid channel that the user can type in ex. the beta-X vs the example formats which include a date (as seen above).
According to the possible argument values I should be able to use latest
however when I do that it tells me that it's an invalid channel.
Overriding an existing toolchain uses the incorrect toolchain. It ignores my input and uses the default now? When I check the content of the file it has beta-4
so it only prints the incorrect toolchain.
I think this should be discussed with @sdankel first. According to the confusion listed above, it seems that you have mixed up the Such as
Reference to the comments by @sdankel
Just as the help message described, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also split the tests a bit further to group them in order to reduce the cognitive load and make it easier to read.
I would opt for either putting all the success cases at the top and panics at the bottom or consider putting the success cases in a mod success
and the panics in a mod panic
.
After the tests are split I'll look over test coverage because I haven't exactly checked if all cases are covered
Addressed! |
This produces a file that looks like this:
But we want it formatted like this:
|
I fixed the formatting issue and added tests for it. However I noticed it still pulls the wrong name for the channel by default. When I'm on beta-3, it pulls the full toolchain name rather than the shortened dist name for the channel. This test should pass:
|
One of the valid channel should be |
What I'm saying is if I'm on beta-3, the toolchain name is |
I'm sorry, but this is expected IMO. As we discussed before, reference to this thread
|
Yes, and we should parse out |
close #322
Support
fuelup toolchain export