-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
warn user when requested extra does not exist - 2138 #2142
Conversation
This is in reference to issue #2138 |
I'm actually confused a little bit about how this is working right now. It appears that Since this code looks like it takes a failure and turns it into a warning I'm guessing that this is only going to take affect for Wheel installs. It would be great to unify this so that we raise the same warning in both cases. |
Sorry about that dstufft, I meant to put a label on the PR that this was in progress. In effect, this PR doesn't do anything right now. I mainly made the PR so that I could use travis. |
@derwolfe: Are you planning to update this? |
@msabramo - yes, I am planning on working on it this week before Friday. |
509c99c
to
acf07b6
Compare
This is related to PR #2153. Once it lands, the job of this PR will (should) be very simple to implement. Most of the legwork is really done by the pkg_resources.Distribution.requires() function, which that PR enables for non-wheel installs. |
Sine PR #2153 has landed, tbis should be pretty simple to implement. I'm on vacation now, but I will be able to start working on this again on Dec 28. |
Need to access the extras defined on the distribution itself. This way I could just check for membership in the set of extras defined, and throw an error for each, instead of stopping entirely.
ade83b9
to
91f8010
Compare
…t exisiting isn't handled
Still working on this, albeit very slowly. I've rebased against develop so the #2153 changes are included. Currently, if a non existent option is passed in and a single exception is thrown, This means if a user has a list of options like pip install twisted[zorro, tls] where tls actually exists; because of zorro failing, the tls option will not be installed. In my mind the correct behaviour should be: try to install every option, if the option doesn't exist, log it, and move on to the next. @dstufft: does this proposed behaviour seem reasonable to you? |
Ready for review :-). |
for subreq in dist.requires( | ||
RequirementSet._available_extras( | ||
dist.extras, | ||
req_to_install.extras)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static methods seem a bit much for basically doing
set(req_to_install.extras) & set(dist.extras)
and
set(req_to_install.extras) - set(dist.extras)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I hadn't though about using sets. I will update the code to use this and will remove the static methods and unit tests covering them. Thanks for your feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xavfernandez - Though, there is an issue with sets not guaranteeing the order of their items. Would a decent compromise be to just inline the comprehensions? This would still remove the static methods and their unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could always sort the set items if you need guaranteed ordering for tests or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True - I could just do sorted(missing)
. Thanks!
I've integrated @xavfernandez's suggested changes into the patch. If anyone can restart the build, it is ready for re-review. |
for missing in missing_requested: | ||
logger.warning( | ||
'%s does not provide the extra \'%s\'', | ||
dist, missing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind if I ask for a bit of style consistency? You have
lval = func(
args
)
And
obj.method(
args,
more_args)
'simplewheel[nonexistant]', expect_stderr=True, | ||
) | ||
assert ( | ||
"simplewheel 2.0 does not provide the extra 'nonexistant'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonexistant => nonexistent
@msabramo - thanks for your comments. I've made the changes you requested. |
Also fixes #852 ? |
Nice.
It's nice that it's a warning instead of a fatal error with a traceback. I did run into a problem where it couldn't figure out the name of the package when I tried it with sentry (the example in #852):
Note how it say "Unknown" instead of "sentry". Although to be fair, the current code on
|
|
OK, I think I found a fix for the above issue. I'll file a separate PR for it. |
@msabramo - Great! When that error is fixed, some of my new tests can be updated as well. |
Here's the PR I mentioned: #2530 |
If I merge the change here with the change in #2530:
then I get correct behavior:
|
Yes. I believe it does. |
warn user when requested extra does not exist - 2138
Working on adding loggers for the pkg_resources.UnknonwExtra exception that can be thrown when a user attempts to install an unknown extra.