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

Fix #4390 Allow --target to override --user #8555

Closed
wants to merge 8 commits into from
Closed

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Jul 7, 2020

Fixes #4390 by allowing explicitly provided --prefix and --target options to overrule implicitly discovered --user.

… options to overrule implicitly discovered --user.
@pypa-bot
Copy link

pypa-bot commented Jul 7, 2020

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@McSinyx
Copy link
Contributor

McSinyx commented Jul 8, 2020

Hi, as an user I find this confusing. If PIP_USER=1 was set, isn't the way to make it work is to provide --no-user?

I think the real issue here is that from the current output it is not immediately obvious that where the option come from, i.e. the error is not descriptive enough. Such meta issue on log/error might need some closer look pip-wise, e.g. I was confused in GH-8530 as well.

@zooba
Copy link
Contributor Author

zooba commented Jul 8, 2020

If PIP_USER=1 was set, isn't the way to make it work is to provide --no-user

Documenting that option in pip help install would be a good start :)

But why should we force users to manually suppress an option they may not have set? We know that they passed --target and did not pass --user, so we can safely infer --no-user.

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 8, 2020
@McSinyx
Copy link
Contributor

McSinyx commented Jul 8, 2020

Documenting that option in pip help install would be a good start :)

I completely agree.

But why should we force users to manually suppress an option they may not have set?

Because we don't know if they have set it or not. To quote the Zen of Python

Explicit is better than implicit.

and TBH I'd rather have an error then potentially running something I'm not completely aware of. IMHO tools should never try to be smart, but rather be consistent and predictive. I have no idea if that's what other users would expect though, so some research from the UX team would be really nice. @ei8fdb and @nlhkabu, do you thing it's worth it to conduct some queries on this?

@zooba zooba changed the title Fix #4390 Allow --prefix/--target to override --user Fix #4390 Allow --target to override --user Jul 8, 2020
@zooba
Copy link
Contributor Author

zooba commented Jul 8, 2020

Because we don't know if they have set it or not.

This PR enables us to tell the difference between options specified on the command line and defaults picked up from the environment or configuration file.

@nlhkabu
Copy link
Member

nlhkabu commented Jul 22, 2020

@McSinyx @zooba - If needed, I can run some research on this in #8516?

@McSinyx
Copy link
Contributor

McSinyx commented Jul 23, 2020

@nlhkabu, some additional input would be great, although I'm not sure if it fits GH-8516, since the argument between @zooba and I here is more about the philosophy of pip's operation: should we error out conflicting options or use some logic imply one of the execution path?

Moreover, as said by @zooba on hidden options

Documenting that option in pip help install would be a good start :)

currently we are having too many options and flags. I think part of the UX work is to tidy this up, but in case there are still many, how will we provide the docs to the users? I think the hidden options like --no-user were deliberately hidden to avoid cluttering the help page—should we think of alternatives that are easier to navigate like texinfo (in addition to online docs) with full docs and keep help minimal? I have many questions I don't have answers to and I'm exited to see them solved in the next few months.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 20, 2021
@pradyunsg
Copy link
Member

It looks like the underlying issue has been fixed, based on the linked issue being closed. Closing this out, since this is significantly out of date.

@pradyunsg pradyunsg closed this Sep 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The --target option clashes with other command line flags and config files
6 participants