cleanup: fix typing in command/build.py #249
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shubhbapna
commented
Jul 25, 2024
src/fromager/wheels.py
Outdated
| @@ -111,7 +111,7 @@ def build_wheel( | |||
| wheels = list(ctx.wheels_build.glob("*.whl")) | |||
| if wheels: | |||
| return wheels[0] | |||
| return None | |||
| raise FileNotFoundError(f"Could not locate the built wheels for {req.name}") | |||
There was a problem hiding this comment.
@dhellmann from what I noticed wherever this function is called, the caller already expects a non null return value. Do you think if we raise an error instead of returning null it would be fine? Or would you rather make this change later?
There was a problem hiding this comment.
Let's do that separately so we can focus on test coverage for it.
There was a problem hiding this comment.
okay then I will fix the type errors from this separately
38 tasks
tiran
reviewed
Jul 26, 2024
src/fromager/wheels.py
Outdated
| @@ -111,7 +111,7 @@ def build_wheel( | |||
| wheels = list(ctx.wheels_build.glob("*.whl")) | |||
| if wheels: | |||
| return wheels[0] | |||
| return None | |||
| raise FileNotFoundError(f"Could not locate the built wheels for {req.name}") | |||
There was a problem hiding this comment.
The signature of the exception class is a bit obscure. It takes an errno number, error string, and file or target name:
Suggested change
| raise FileNotFoundError(f"Could not locate the built wheels for {req.name}") | |
| raise FileNotFoundError(errno.ENOENT, "Could not locate the built wheels for", req.name) |
>>> import errno
>>> from packaging.requirements import Requirement
>>> req = Requirement("egg==0.1")
>>> raise FileNotFoundError(errno.ENOENT, "Could not locate the built wheels for", req.name)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] Could not locate the built wheels for: 'egg'
tiran
requested changes
Jul 26, 2024
Will be adding the exception call in a separate PR. Have noted this down |
shubhbapna
force-pushed
the
typing-command-build
branch
from
53a1d71 to
56c042b
Compare
shubhbapna
force-pushed
the
typing-command-build
branch
from
927747f to
7d147cb
Compare
tiran
approved these changes
Jul 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
part of #226
fixes