Replacing filters .format() calls with f-strings #5478
Conversation
| msg = (f'Window size for `threshold_sauvola` or ' | ||
| f'`threshold_niblack` must not be even on any dimension. ' | ||
| f'Got {axis_sizes}') |
There was a problem hiding this comment.
The first two lines here don't have to have the f since there is no substitution there, but I don't think it hurts. I don't know if it is more common to leave it off in that case as below? Maybe @mkcor has an opinion?
msg = ('Window size for `threshold_sauvola` or '
'`threshold_niblack` must not be even on any dimension. '
f'Got {axis_sizes}')There was a problem hiding this comment.
@grlee77 Yup, that should be fine as well. As far as I can tell, I don't think that there's too much out there in terms of style guidelines for this - I had left it this way after reading the PEP and a few posts like this one, as it felt more readable to me and less likely to lead to wrong messages if they were changed in the future.
I don't feel strongly about it though and am happy to change it if you'd prefer the lines without placeholders to be regular strings
There was a problem hiding this comment.
Good question! At first I thought: Why bother with an extra, unnecessary f prefix? I like to follow a minimalist approach. But after reading the SO post @alex-jw-brooks linked to, I'm happy to go for the multi-line f prefix!
| msg = (f'Window size for `threshold_sauvola` or ' | ||
| f'`threshold_niblack` must not be even on any dimension. ' | ||
| f'Got {axis_sizes}') |
There was a problem hiding this comment.
Good question! At first I thought: Why bother with an extra, unnecessary f prefix? I like to follow a minimalist approach. But after reading the SO post @alex-jw-brooks linked to, I'm happy to go for the multi-line f prefix!
That is good to hear! If it is not too much hassle can you split it in two as suggested above (f-strings vs. other single quote changes)? We could merge the f-string part now and leave the other up for a day or two for potential comment. |
alex-jw-brooks
force-pushed
the
filters-fstrings
branch
from
ba04cac
to
593b719
Compare
|
Sounds good, thanks @grlee77 and @mkcor - I went ahead and updated the PR to only include the original changes and the style changes on the f-strings from the review. I will plan to open a separate PR in the future for moving to single quotes on one of the packages to gather some feedback from the team to see if this is something in general most people would feel good about standardizing on for the rest of the library |
| stacklevel=2) | ||
|
|
There was a problem hiding this comment.
Unintentional - added it back. Nice catch, and thank you for the quick reviews! 😄
Clarify multiotsu thresholding error message Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
|
Thanks for your patience and responsiveness here @alex-jw-brooks. It is now merged! |
Description
This PR replaces
.format()calls on strings within thefilterspackage with f-strings to partially address issue #5474.Thank you!