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

Allow empty specifier in constraints #8209

Closed
wants to merge 1 commit into from

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented May 8, 2020

A test test_constrained_to_url_install_same_url is currently failing because it supplies a constraint with an empty specifier, which evaluates to false, failing the assertion. (A constraint with an empty specifier won’t actually do anything, so why is this allowed…? But apparently it is allowed.)

InstallRequirement’s type hints say req.specifier is always SpecifierSet, i.e. the assertion is not needed, so I simply dropped it.

@uranusjr uranusjr requested a review from pfmoore May 8, 2020 21:36
@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label May 8, 2020
@uranusjr
Copy link
Member Author

uranusjr commented May 8, 2020

Oh wait, I’m checking test_install_distribution_union_with_constraints and it seems that even the name of a constraint is not guarenteed to exist. Something like this is actually valid:

https://github.com/psf/requests/archive/master.zip[socks]

@uranusjr
Copy link
Member Author

uranusjr commented May 8, 2020

Err, the nameless constraint case is much more difficult to deal with. I’ve opened #8210 to discuss this further, but this one can probably go in first.

@pfmoore
Copy link
Member

pfmoore commented May 15, 2020

This got covered in my constraint fixup PR, so I don't think it's needed any more.

@uranusjr uranusjr closed this May 15, 2020
@uranusjr uranusjr deleted the constraint-specifier-empty branch May 15, 2020 16:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants