-
-
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
Clean up tests #6653
Clean up tests #6653
Conversation
Thanks for this PR! The first five commits look good. "use tuple or set instead of list in tests" is too big to review properly, and I know tuples are generally preferred to lists, but in this case I don't think the huge review effort is worth any noticeable benefit. (I think these kind of changes are okay when making other changes to a file.) And @radarhere had a question about "sort colors before comparing them", so I'll defer on that. Also I wouldn't worry about the last few commits splitting long lines and wrangling with the formatting, it's okay to have longish strings (below the limit) as they're easier to grep. |
@@ -57,13 +57,13 @@ def test_convolution_modes(self): | |||
Image.Resampling.LANCZOS, | |||
), | |||
) | |||
def test_reduce_filters(self, resample): | |||
r = self.resize(hopper("RGB"), (15, 12), resample) |
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.
Why is 'resampling_filter' clearer? 'resample' is the name of the variable when the core resize operation is called in Image.py
Line 2192 in 86634b8
return self._new(self.im.resize(size, resample, box)) |
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.
Because "resample" is an action, while "resampling filter" is an object. The thing being passed is (an ID for) a resampling filter, not a "resample". It doesn't work to say that "resample" is the type of the thing being passed either, because the type of the thing being passed is a Resampling
enum item.
Those PRs have all now been merged. |
Assert statements give a better message this way.
I've rebased it. Should I create a new pull request for this, or keep using this one? |
This PR is fine. I don't think we really need 3f045ea "use tuple or set instead of list in tests", there's a lot of changes (+406 −399 in 54 files) for little benefit. |
I've moved the |
No description provided.