CLN: String formatting % -> f-strings #29518
Conversation
pandas/core/arrays/period.py
Outdated
| formatter = lambda dt: dt.strftime(date_format) | ||
| else: | ||
| formatter = lambda dt: "%s" % dt | ||
| formatter = lambda dt: f"{dt}" |
There was a problem hiding this comment.
would str(dt) be clearer here?
| return None | ||
| else: | ||
| if verbose: | ||
| print("unable to find command, tried %s" % (commands,)) |
There was a problem hiding this comment.
i think this file is auto-generated
| index=index, | ||
| partition_cols=partition_cols, | ||
| **kwargs | ||
| **kwargs, |
There was a problem hiding this comment.
is this a black version thing?
There was a problem hiding this comment.
Weirdly using 19.3b0 locally seems to add this for me - I've reverted
There was a problem hiding this comment.
Black code check in CI fail without this - i've added this back in
There was a problem hiding this comment.
Personally, I find this quite odd that you have to add a comma at the end.
|
looks like black complaints |
|
@jbrockmendel - ready to re-review whenever you have a moment |
pandas/core/indexes/base.py
Outdated
| space = self._format_space() | ||
|
|
||
| prepr = (",%s" % space).join("%s=%s" % (k, v) for k, v in attrs) | ||
| prepr = f",{space}".join(f"{k}={v}" for k, v in attrs) |
There was a problem hiding this comment.
let's try to minimize what we execute in f-space (better name for this?)
pandas/core/indexes/base.py
Outdated
| if hasattr(tail, "format") and not isinstance(tail, str): | ||
| tail = tail.format() | ||
| index_summary = ", %s to %s" % (pprint_thing(head), pprint_thing(tail)) | ||
| index_summary = f", {pprint_thing(head)} to {pprint_thing(tail)}" |
There was a problem hiding this comment.
pprint_thing may be unnecessary here now that we're py3-only. can you take a look
| ) | ||
|
|
||
| freq = "%dL" % self._get_interval() | ||
| freq = f"{self._get_interval()}L" |
There was a problem hiding this comment.
call get_intterval outside
pandas/core/indexes/datetimelike.py
Outdated
| freq = self.freqstr | ||
| if freq is not None: | ||
| freq = "'%s'" % freq | ||
| freq = f"'{freq}'" |
There was a problem hiding this comment.
do the single quotes become unnecessary with a {freq:r} or something like that? This is a pretty common pattern, but we aren't very consistent about when we do or dont use the quotes (i sometimes use ticks like for markdown)
There was a problem hiding this comment.
Assuming you mean {freq!r} here - updated. Agree doesn't seem to be much consistency
|
A handful of comments, general rule about minimizing the code that is executed inside f-space. If its viable to have the black-version-related stuff not in this diff that'll make it a little easier. cc @gfyoung for another look |
|
Looks pretty good! I agree with the comments here. |
|
Updated as per comments and green cc. @jbrockmendel and @gfyoung |
|
thanks @alimcmaster1 |
4 participants
Follow up from CI: Drop Python 3.5 support #29212
Towards Replace old string formatting syntax with f-strings #29547
passes
black pandaspasses
git diff upstream/master -u -- "*.py" | flake8 --diffOld style % formatting to f-strings