-
-
Notifications
You must be signed in to change notification settings - Fork 597
fix: Warn only when all SHAs fail to match a requirement #2922
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
base: main
Are you sure you want to change the base?
fix: Warn only when all SHAs fail to match a requirement #2922
Conversation
7238ba3
to
7eb4977
Compare
@@ -334,8 +334,8 @@ def _add_dists(*, requirement, index_urls, logger = None): | |||
sdist = maybe_sdist | |||
continue | |||
|
|||
if logger: | |||
logger.warn(lambda: "Could not find a whl or an sdist with sha256={}".format(sha256)) | |||
if logger and (len(whls) == 0 and sdist == None): |
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 vars are undefined here. What is that you are trying to achieve?
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 vars are undefined here.
@aignas maybe I'm missing something, but whls
& sdist
appear to be defined at:
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.
Oh sha256
in the log call isn't defined, let me fix that.
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.
Ah sorry, somehow I missed it.
However, the intention of the check though was to tell the users that something is wrong andthat the lock file contains hashes that we could not find on the index.
What would be the use case where such behaviour is not desired?
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.
Ping
I believe the missing requirement logging is over reporting when a requirement has multiple SHAS and not all match.
That being said, It's possible I'm misunderstanding and this warning is working as intended.