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

7122: error on encountering invalid extras #12987

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

meejah
Copy link

@meejah meejah commented Oct 2, 2024

WIP: report an error and exit with code 1 upon encountering an invalid exit code.

Requires tests, at least, if this is an acceptable solution to #7122 .

@meejah meejah force-pushed the 7122.error-invalid-extras branch from 7f16323 to 24253bc Compare October 2, 2024 21:42
@notatallshaw
Copy link
Member

notatallshaw commented Oct 2, 2024

As discussed in that issue this behavior would have to change over at least a couple of releases, with an opt in, and deprecation warning, as it was considered too disruptive 5 years ago, it would be even more disruptive now.

An important behavior also would be if I request foo[bar] and foo 2.0 did not have the extra bar but foo 1.0 did have the extra bar, does this throw an exception or does it select foo 1.0 with the extra bar? I would strongly prefer the later behavior.

@meejah
Copy link
Author

meejah commented Oct 2, 2024

An important behavior also would be if I request foo[bar] and foo 2.0 did not have the extra bar but foo 1.0 did have the extra bar, does this throw an exception or does it select foo 1.0 with the extra bar? I would strongly prefer the later behavior.

That seems like a separate issue, maybe?

(I suggest a separate issue because before this change, it would have still silently installed foo 2.0 with no error).

@meejah
Copy link
Author

meejah commented Oct 2, 2024

As discussed in that issue this behavior would have to change over at least a couple of releases, with an opt in, and deprecation warning, as it was considered too disruptive 5 years ago, it would be even more disruptive now.

Okay, so this means adding an option to pip install in order to opt in? Not sure how to spell this, maybe pip install --error-extras or...?

I'm not sure what you mean by deprecation warning? Do you mean that the "warning" text I need to put back (if the user doesn't include --error-extras) should also say something like "this warning is deprecated and will become an error in a future release"?

@notatallshaw
Copy link
Member

notatallshaw commented Oct 2, 2024

That seems like a separate issue, maybe?

(I suggest a separate issue because before this change, it would have still silently installed foo 2.0 with no error).

Currently 2.0 is installed, and this behavior has existed for so long people rely on this (Hyrum's law). Which is why this would need to be changed over multiple releases.

But if this PR throws an exception when there is a valid candidate (1.0) many users will (possibly rightfully) loudly complain.

@meejah
Copy link
Author

meejah commented Oct 2, 2024

But if this PR throws an exception when there is a valid candidate (1.0) many users will (possibly rightfully) loudly complain.

I believe users may complain either way. I would certainly not want that behavior -- I would want the error, unless I said something like "foo[bar]==1.0" or otherwise specified an older version (moving this part of the discussion back to the actual bug ticket though, see #7122 (comment) )

@notatallshaw
Copy link
Member

I believe users may complain either way. I would certainly not want that behavior -- I would want the error, unless I said something like "foo[bar]==1.0" or otherwise specified an older version

But this is contrary to analogues behavior, if you requested foo and there was 2.0 and 1.0, but 2.0 wasn't available for your platform you get 1.0 not an error.

@notatallshaw
Copy link
Member

notatallshaw commented Oct 3, 2024

Okay, so this means adding an option to pip install in order to opt in? Not sure how to spell this, maybe pip install --error-extras or...?

Yeah, like --use-pep517, it affects more than pip install ..., it will affect at least pip download, pip wheel, perhaps others. I'm not opinionated on the name, I'm sure people can bikeshed that.

I'm not sure what you mean by deprecation warning? Do you mean that the "warning" text I need to put back (if the user doesn't include --error-extras) should also say something like "this warning is deprecated and will become an error in a future release"?

Here's a recent example of adding a deprecation warning: #12918

Very informally the warning should be something like "invalid extra found, in the future this requirement will not be accepted, opt into the new behavior with option {new option}."

@notatallshaw
Copy link
Member

Also, FYI, CI is currently broken until #12964 is merged.

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

Successfully merging this pull request may close these issues.

2 participants