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

Add --unstable-feature=resolver #7845

Merged
merged 1 commit into from
Mar 11, 2020
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Mar 11, 2020

This introduces a new general option --unstable-feature that can be used to opt into “preview” features in pip not enabled by default. Currently the only available feature is resolver.

Based on #5727, but with a more bare-bone implementation since we have only one user (the resolver) at the moment. Refactoring can always come later :p

The --unstable-feature option is hidden from --help since the resolver does not yet work. This suppression should be removed when we release the resolver for general/public testing. I am also intentionally not providing a news fragment for the same reason—that can be added when we lift the help message suppression.

A stub resolver interface (which would fail on invocation) is provided to respond to the flag. Like #7843, this can be merged standalone, and implementation can be added after other PRs are added to flesh it out.

Fix #7603.

@uranusjr
Copy link
Member Author

cc @pradyunsg @pfmoore

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

LGTM

@uranusjr
Copy link
Member Author

Mypy is so hard to please 😞

@pfmoore
Copy link
Member

pfmoore commented Mar 11, 2020

Harder than I am, apparently 😃

@uranusjr uranusjr force-pushed the resolver-flag branch 3 times, most recently from 3a8bb66 to 3730aa0 Compare March 11, 2020 10:38
This introduces a new general option --unstable-feature that can be used
to opt into "preview" features in pip not enabled by default. Currently
the only available feature is "resolver".

A stub resolver interface (which would fail on invocation) is provided
to respond to the flag.

The --unstable-feature option is hidden from --help since the resolver
does not yet work. This suppression should be removed when we release
the resolver for general/public testing.
@pfmoore
Copy link
Member

pfmoore commented Mar 11, 2020

Note: This now has merge conflicts with #7843. That's not a problem (they are shallow conflicts) but IMO we should get some of these enabling PRs merged ASAP, so that we can build on them rather than continually revising WIP PRs.

@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Mar 11, 2020
@pfmoore
Copy link
Member

pfmoore commented Mar 11, 2020

I'm going to merge this and then rebase and merge #7843

We can iterate on either of them later, so this will keep things from getting blocked.

@pfmoore pfmoore merged commit f981fac into pypa:master Mar 11, 2020
@pradyunsg
Copy link
Member

Sounds reasonable to me!

]


class BaseResolver(object):
Copy link
Member

Choose a reason for hiding this comment

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

nit: add_metaclass using six and use abc.ABCMeta?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I dislike abc.ABCMeta in general since it adds unnecessary runtime overhead. But I’d be happy to fix this if there is a strong project preference to use it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to leave this as it is. I don't particularly see much value in ABCMeta.

Copy link
Member

Choose a reason for hiding this comment

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

It was nit anyway, either works for me too!

@ei8fdb
Copy link
Contributor

ei8fdb commented Mar 12, 2020

@uranusjr is this syntax final? Is there a possibility of it being changed?

@uranusjr
Copy link
Member Author

@ei8fdb It is unless you propose a change!

@ei8fdb
Copy link
Contributor

ei8fdb commented Mar 12, 2020

@uranusjr I'd like to try. I was just wondering if its possible to change. Are there any restrictions on the type of syntax I can propose?

@uranusjr
Copy link
Member Author

uranusjr commented Mar 12, 2020

Not many. The usual restrictions of a flag apply (must start with a - etc.), and there are some alternative proposals in #7603. The original implementation for this flag (#5727) also proposed a shorthand -X, which I omitted because I wanted to avoid committing to the limited single-character namespace before we are sure this flag is a good idea.

@uranusjr uranusjr deleted the resolver-flag branch March 19, 2020 13:46
@uranusjr uranusjr mentioned this pull request Apr 3, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Apr 18, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation 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.

Add a command line option to select the resolver implementation
4 participants