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

Alternate short channel matching support #838

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

christiangnrd
Copy link
Contributor

Closes #836 and closes #830.

If the channel is the prefix for one and only one installed channel, use that, otherwise old behaviour. This behaves more like the julia command line arguments

Should work with any installed channel including linked channels instead of a hard-coded few.

@christiangnrd christiangnrd force-pushed the short-channel-match branch from f5f36cb to 8d70142 Compare May 29, 2024 19:09
@christiangnrd christiangnrd force-pushed the short-channel-match branch 2 times, most recently from cfbbe53 to e326367 Compare July 3, 2024 13:19
@LilithHafner
Copy link
Member

I don't like this because I could have Julia 1.6.2 as the only installed version and then run julia +1 and get that version. Similarly, I could have Julia 1.10 installed and run julia +1.1 and get version 1.10 instead.

The reasons this short matching works okay in Julia (and some other programs') command matching is that

  • Their options are intelligently named to avoid misleading prefixing
  • Their options are words or phrases that have a more diverse prefix set than version numbers

Whereas in this use case,

  • Juliaup's set of version options varies depending on the installation (if we are filtering to installed channels)
  • Juliaup's option names are already very concise so the benefit is less.

@christiangnrd
Copy link
Contributor Author

christiangnrd commented Jul 3, 2024

I've updated to not match on version numbers. The approach feels hacky but also I don't know if it needs to be more complicated.

Unfortunately the new tests end up downloading 2 extra Julia versions so that might take up a lot more time. Maybe I should test with channels linked to already downloaded versions?

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

This works; I left a few comments, but fundamentally it's a design choice between this and #836. Do we want +r to always be release, or to be either rc or release depending on which channels the use has installed. I lean toward the simplicity of #836.

src/bin/julialauncher.rs Outdated Show resolved Hide resolved
src/bin/julialauncher.rs Outdated Show resolved Hide resolved
src/bin/julialauncher.rs Outdated Show resolved Hide resolved
tests/channel_selection.rs Outdated Show resolved Hide resolved
tests/channel_selection.rs Outdated Show resolved Hide resolved
Comment on lines 137 to 138
.arg("-e")
.arg("print(VERSION)")
Copy link
Member

Choose a reason for hiding this comment

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

These are also unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a stupid question but how does the test not get stuck in the REPL without the "-e"?

Copy link
Member

Choose a reason for hiding this comment

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

Good point; that is necessary (or -v or something)

Copy link
Contributor Author

@christiangnrd christiangnrd Jul 4, 2024

Choose a reason for hiding this comment

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

I was asking because the tests pass without them and I was genuinely wondering what made it work. I'll add -v to be safe though.

@christiangnrd
Copy link
Contributor Author

christiangnrd commented Jul 3, 2024

@LilithHafner Thank you very much for your review! I've addressed your feedback and implemented all your suggestions.

Regarding this PR or #836, I wrote this because I sometimes write more than just the first letter, and I want this feature to work with linked channels. Also, for someone like me who sometimes tries out PRs but rarely more than one at a time, once #903 is a thing, it'll be nice to be able to call julia +pr instead of having to remember the pull request number.

@LilithHafner
Copy link
Member

@rakeshksr @christiangnrd and espeically @davidanthoff, could we discuss which option we prefer (#838 vs #836) both are imo ready to merge but we need to pick which.

@christiangnrd
Copy link
Contributor Author

christiangnrd commented Aug 5, 2024

My justification for preferring this approach is in my previous comment. To add to that, I always have a dev channel linked to my local branch of Julia, I feel like it would be quite unintuitive to have +b work for beta, and +l work for lts, but +d error out for dev.

@christiangnrd christiangnrd force-pushed the short-channel-match branch 6 times, most recently from e898dbf to 20b739d Compare August 21, 2024 16:22
@christiangnrd christiangnrd force-pushed the short-channel-match branch 3 times, most recently from 8d04e4b to 1eebbd4 Compare September 6, 2024 21:22
@davidanthoff
Copy link
Collaborator

I think I've finally am getting clarity what I'm thinking about this, see #830.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support short channel matching
3 participants