-
Notifications
You must be signed in to change notification settings - Fork 102
Add alias
command
#1115
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
Add alias
command
#1115
Conversation
7be2505
to
90cbffd
Compare
1459d43
to
0957f01
Compare
Seems good to me (very naively). Could we get a rebase and a review? |
Bump. @davidanthoff, could you have a look at this or suggest someone else who could review? |
@IanButterworth, you've got some Rust chops. Could you review this and merge if it looks good? |
I got Claude to help a review and found a few issues that I tested and fixed in #1235
I would set the merge base of that PR to this one, but your branch is on a fork so I can't do that. @christiangnrd let me know what you think of #1235. I can push them here if you agree. Otherwise this is looking good to me. |
@IanButterworth Aside from the two comments I made in #1235, the rest looks great. Feel free to push here. Thank you for taking the time to review this! |
Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
Actually wait. Before merging a hand rolled version, does rustup have something new for this we could pull? The more updated we can be with upstream the better in my view. |
Co-authored-by: Christian Guinard <[email protected]>
Looks like rustup added a similar feature upstream called a "toolchain override shorthand" I feel like it would be better to merge features from upstream rather than recreate them from scratch |
I'm not familiar with rustup but that doesn't seem equivalent? It doesn't seem to provide a way to alias a channel (or toolchain) just shows ways to use different standard toolchains, which is what juliaup already provides. Aliasing channels seems different? |
That seems to be the |
Sorry I confused myself that juliaup isn't a literal fork of rustup, it was just heavily inspired in its design. So we can't "merge" features per se from "upstream" But given the similarities in designs, I think it would be good to stay close to rustup's design choices unless there's a strong reason not to for something. A rustup "toolchain" is analogous to a juliaup "channel." A rustup "override" means simply passing a So a "toolchain override shorthand" sounds like an alias (though I don't see much in the form of docs on that page?). Anyways I think it would be good for us to look at that design first before settling on a design from scratch here |
Ah, I'm being an idiot, a "shorthand" on that page literally just means the Time for bed... |
I think that's all a different thing. This just making it so you can shorthand. So instead of
you can
|
Oh I see. What's the advantage over I guess I could see a use if #1230 merges, then you could use the alias in different contexts? |
Just pushed a tweak to messaging when removing an alias Was
Now
|
I like this. Should custom channels from Kind of unrelated: Probably out of scope for this PR, since the behaviour existed before this PR (same thing happens when removing linked channels), but I think the "Nothing to remove" should be printed only when I can open an issue from this comment if more discussion is needed. |
Ok, looking at this with fresh eyes; apologies for the confusion earlier. So, I may be missing something, but juliaup link b ~/.julia/bin/julia-beta
# Creating shim julia-b for /Users/mcranmer/.julia/bin/julia-beta .
julia +b # works fine If the issue is usability, could we improve e.g., maybe juliaup link b beta rather than juliaup link b --channel beta Edit: I see @davidanthoff suggested something similar: #830 (comment) juliaup link b +beta which is also my preference |
|
|
@christiangnrd given this PR is old, I thought it might be easier for all to just start afresh with the switch to |
I'm proposing we move forward with #1237 |
Closing in favour of #1237 |
Many implementation decisions were made. I'm open to suggestions/improvements/bikeshedding.
Close #838
Close #830