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. |
| ), | ||
| ) | ||
| def test_reduce_filters(self, resample): | ||
| r = self.resize(hopper("RGB"), (15, 12), resample) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.