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.

Sign up for GitHub

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

Jump to bottom

Add --modify-path and --no-modify-path to self install #664

Merged
merged 4 commits into from
Feb 17, 2024
Merged

Add --modify-path and --no-modify-path to self install #664

merged 4 commits into from
Feb 17, 2024

Conversation

dsp
Copy link
Contributor

@dsp dsp commented Feb 16, 2024 •
edited
Loading

This is an RFC for how we handle skipping certain steps independent
from --yes being present. The PR adds the --modify-path and
--no-modify-path flag that can be used to turn off and on path
modification without prompting the user. his allows people to do
self-installs such as rye self install --yes --no-modify-path.

The goal here is to get fine grained control over the install process
for headless/non-interactive installations. We use --x, --no-x style
flags inline with popular tools like curl, ripgrep, git, etc.

This should address #620.

@dsp dsp marked this pull request as draft February 16, 2024 13:43
@dsp dsp force-pushed the dsp/perform-install branch from e911af1 to 93c2395 Compare February 16, 2024 13:47
@dsp dsp marked this pull request as ready for review February 16, 2024 13:51
@dsp dsp force-pushed the dsp/perform-install branch 2 times, most recently from 378e91e to d97a5f3 Compare February 16, 2024 14:16
@dsp dsp changed the title Add --skip-path to self install Add --skip-modify-path to self install Feb 16, 2024
@mitsuhiko
Copy link
Collaborator

I wonder if it would not be better to just make all these branches arguments.

--modify-path=yes | --modify-path=no | --modify-path=prompt and that for all branches. Then at least this is consistent. And --yes just defaults to yes for all of these.

mitsuhiko
mitsuhiko reviewed Feb 17, 2024
View reviewed changes
rye/src/cli/rye.rs
@@ -597,17 +628,12 @@ fn perform_install(
echo!("For more information read https://mitsuhiko.github.io/rye/guide/installation");
}
}
// On Windows, we add the rye directory to the user's PATH unconditionally.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think at this point it might also sense to align the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can follow up with this in a separate PR. This way we separate the concerns a bit and can merge one but not the other if we feel like it.

@dsp
Copy link
Contributor Author

dsp commented Feb 17, 2024

I wonder if it would not be better to just make all these branches arguments.

--modify-path=yes | --modify-path=no | --modify-path=prompt and that for all branches. Then at least this is consistent. And --yes just defaults to yes for all of these.

I think the more common approach would be providing --no-modify-path and --modify-path. This seems to be the most common pattern I've seen and would be inline with how click, [curl](https://curl.se/docs/manpage.html], ripgrep and to some degree git (these were the onces i checked), handle on-off flags.

Happy to implement whatever you prefer.

@mitsuhiko
Copy link
Collaborator

We can do that, and if not passed it defaults to prompt. Works fo rme.

@dsp dsp force-pushed the dsp/perform-install branch 2 times, most recently from 463c61d to 07c5547 Compare February 17, 2024 15:06
@dsp dsp changed the title Add --skip-modify-path to self install Add --modify-path and --no-modify-path to self install Feb 17, 2024
@dsp dsp force-pushed the dsp/perform-install branch 2 times, most recently from 4180e41 to ed9d74c Compare February 17, 2024 15:17
dsp added 4 commits February 17, 2024 19:44
@dsp
move PATH modification into it's own fn
a667b9f
@dsp
add comments to add_rye_to_path
678d79b
@dsp
add --modify-path and --no-modify-path to rye self install
b611fa8 
This adds the option to never modify paths to rye install, even
when `--yes` is provided. This addresses astral-sh#620.

We offer a --modify-path and a --no-modify-path option to either
always modify or never modify. Providing --yes, will be the same
as --modify-path. This is in line with other command line tools
like curl, git and ripgrep.

We do this by adding a new enum YesNoArg, which can be obtained by
providing an ArgGroup with respective modify_path and no_modify_path
boolean values.
@dsp
fix unused variable warnings on windows
4fcf897 
On windows the function doesn't use all variables. Use cfg_attr to
allow unused_variables.
@dsp dsp force-pushed the dsp/perform-install branch from ed9d74c to 4fcf897 Compare February 17, 2024 19:45
@mitsuhiko mitsuhiko merged commit d950ca2 into astral-sh:main Feb 17, 2024
6 checks passed
@mitsuhiko
Copy link
Collaborator

Thank you!

@dsp dsp deleted the dsp/perform-install branch February 17, 2024 22:42
j178 pushed a commit to j178/rye that referenced this pull request Feb 18, 2024
@dsp @j178
Add --modify-path and --no-modify-path to self install (astral-sh#664)
10bc236
@eifinger eifinger mentioned this pull request Feb 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitsuhiko mitsuhiko mitsuhiko left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dsp @mitsuhiko