-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added type hints to Tests/helper.py #7733
Conversation
Tests/helper.py
Outdated
assert a.mode == b.mode, msg or f"got mode {repr(a.mode)}, expected {repr(b.mode)}" | ||
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
@@ -125,55 +130,68 @@ def assert_image_similar(a, b, epsilon, msg=None): | |||
+ f" average pixel value difference {ave_diff:.4f} > epsilon {epsilon:.4f}" | |||
) | |||
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.
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 <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
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.