-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Added type hints to Tests/helper.py #7733
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
Conversation
Tests/helper.py
Outdated
| assert a.size == b.size, msg or f"got size {repr(a.size)}, expected {repr(b.size)}" | ||
| if a.tobytes() != b.tobytes(): | ||
| if HAS_UPLOADER: | ||
| if uploader: |
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.
This if is probably no longer needed since it is now checked in the upload function.
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.
If this condition was removed, then the logger.error on line 97 would always run, and similarly, the logger.exception on line 136 would always run.
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.
Perhaps it shouldn't run if there is no url returned?
If uploader = "show", that logger message is probably also undesirable.
url = upload(a, b)
if url:
logger.error("URL for test images: %s", url)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.
Ok, I've pushed f7701e6 for this.
Tests/helper.py
Outdated
| ) | ||
| except Exception as e: | ||
| if HAS_UPLOADER: | ||
| if uploader: |
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.
Same for this if.
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.
Ok, I've pushed f7701e6 for this.
hugovk
left a comment
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 also added some hints to this file yesterday but hadn't opened the PR yet :) The good news is we can combine these!
13f626f to
f7701e6
Compare
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Hugo van Kemenade <[email protected]>
| elif uploader == "aws": | ||
| return test_image_results.upload(a, b) |
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.
Out of scope for this PR, but if the AWS Lambda is long gone, shall we remove this stuff?
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.
Ok, I've created #7739 to remove it.
No description provided.