Replace most str.format() uses with f-strings
#5337
Conversation
Along the way, I've made some small code improvements beyond just the use of f-strings; I don't think these will conflict with others' work. In particular, I've avoided modifying the formatting of paths; there is work to greatly simplify that using 'pathlib', at which point a second commit can clean them up.
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
| return f"{m.group(2)} {m.group(1)}" | ||
|
|
||
| str1 = re.sub(SD_END_REPLACE, replacer, str1) | ||
| str2 = re.sub(SD_END_REPLACE, replacer, str2) | ||
|
|
There was a problem hiding this comment.
Neither the old option nor your version is incredibly readable. However I think I prefer the old version, because "endswith" is more readable. Could you maybe change var m to something more descriptive? And maybe change replacer to something that sounds more like a function? Like do_replace?
There was a problem hiding this comment.
That's fair. I think a more intuitive implementation would be based on str.rsplit() or str.rpartition() using , as a delimiter, and checking that the final component is one of the three words. If that doesn't sound good, I can improve what I have here: m -> match_info and replacer -> move_article_to_front?
There was a problem hiding this comment.
I disagree with @RollingStar actually, I think your regex is clearer. Maybe I'm just more familiar/confident with regex? One thing I would note is that it might be better, if we stay with this approach, to make the regex case-insensitive.
| setup_sql += "ALTER TABLE {} ADD COLUMN {} {};\n".format( | ||
| table, name, typ.sql | ||
| setup_sql += ( | ||
| f"ALTER TABLE {table} ADD COLUMN {name} {typ.sql};\n" | ||
| ) |
There was a problem hiding this comment.
Flagging here that we always want to be attentive when changing SQL commands.
| @@ -151,6 +151,7 @@ def __init__(self, field_name: str, pattern: P, fast: bool = True): | |||
| self.fast = fast | |||
|
|
|||
| def col_clause(self) -> Tuple[str, Sequence[SQLiteType]]: | |||
| # TODO: Avoid having to insert raw text into SQL clauses. | |||
|
Thank you for the monster commit. Hopefully you had automation to aid you. I only looked at the first few chunks (about as far as my final comment) but it passes the smell test. I would prefer having the SQL changes in their own commit. Anything with SQL should probably have more attention on it than an f-string. |
Thanks for reviewing! This is a pretty big change and I am worried I slipped up somewhere, so I'm happy to hang on to the PR until somebody can look at the full diff. PyCharm helped me find the usages of
That's understandable, messing with SQL can be dangerous. The tests pass, which I hope adds some degree of confidence to these changes. I haven't fundamentally changed any SQL statements, though -- they should operate exactly the same as before. I can refactor them out to a separate commit, but I think that should wait until somebody reviews the entire diff. |
|
I'll review the entire diff and get back to you. |
beets/autotag/mb.py
Outdated
| return "{} in {} with query {}".format( | ||
| self._reasonstr(), self.verb, repr(self.query) | ||
| ) | ||
| return f"{self._reasonstr()} in {self.verb} with query {self.query!r}" |
There was a problem hiding this comment.
I agree with the person here in the PEP for this, I think !r is better replaced with repr(self.query). It's more expressive and it's easier to tell what is happening.
There was a problem hiding this comment.
Ah, right! I saw that !r / !s were a thing and figured they were the nicest way of implementing this. Apparently that's not the case -- and you're right, it makes the behavior of the code less visible -- so I'll change all of them.
| + "# ====== {} =====".format("completions for " + name) | ||
| + "\n" | ||
| ) | ||
| word += "\n\n" f"# ====== completions for {name} =====" "\n" |
| url = urljoin( | ||
| "{}://{}:{}".format(get_protocol(secure), host, port), api_endpoint | ||
| ) | ||
| url = urljoin(f"{get_protocol(secure)}://{host}:{port}", api_endpoint) |
| url = urljoin( | ||
| "{}://{}:{}".format(get_protocol(secure), host, port), api_endpoint | ||
| ) | ||
| url = urljoin(f"{get_protocol(secure)}://{host}:{port}", api_endpoint) |
beets/dbcore/db.py
Outdated
| """ | ||
| if fields is None: | ||
| fields = self._fields | ||
| fields = self._fields.keys() | ||
| fields = set(fields) - {"id"} | ||
| db = self._check_db() | ||
|
|
||
| # Build assignments for query. | ||
| assignments = [] | ||
| subvars = [] | ||
| for key in fields: | ||
| if key != "id" and key in self._dirty: | ||
| self._dirty.remove(key) | ||
| assignments.append(key + "=?") | ||
| value = self._type(key).to_sql(self[key]) | ||
| subvars.append(value) | ||
| dirty_fields = list(fields & self._dirty) | ||
| self._dirty -= fields | ||
| assignments = ",".join(f"{k}=?" for k in dirty_fields) | ||
| subvars = [self._type(k).to_sql(self[k]) for k in dirty_fields] | ||
|
|
||
| with db.transaction() as tx: | ||
| # Main table update. | ||
| if assignments: | ||
| query = "UPDATE {} SET {} WHERE id=?".format( | ||
| self._table, ",".join(assignments) | ||
| ) | ||
| query = f"UPDATE {self._table} SET {assignments} WHERE id=?" | ||
| subvars.append(self.id) | ||
| tx.mutate(query, subvars) | ||
|
|
||
| # Modified/added flexible attributes. | ||
| for key, value in self._values_flex.items(): | ||
| if key in self._dirty: | ||
| self._dirty.remove(key) | ||
| tx.mutate( | ||
| "INSERT INTO {} " | ||
| "(entity_id, key, value) " | ||
| "VALUES (?, ?, ?);".format(self._flex_table), | ||
| (self.id, key, value), | ||
| ) | ||
| flex_fields = set(self._values_flex.keys()) | ||
| dirty_flex_fields = list(flex_fields & self._dirty) | ||
| self._dirty -= flex_fields | ||
| for key in dirty_flex_fields: | ||
| tx.mutate( | ||
| f"INSERT INTO {self._flex_table} " | ||
| "(entity_id, key, value) " | ||
| "VALUES (?, ?, ?);", | ||
| (self.id, key, self._values_flex[key]), | ||
| ) |
There was a problem hiding this comment.
This does result in the same values, but not the same ordering because of the conversion to sets. If that will cause problems should be double and triple-checks since this is ultimately being made into SQL commands.
There was a problem hiding this comment.
Oh, I forgot about the order changing! I don't think it'll cause issues, but I can actually think of a nicer layout for this implementation in terms of not in that does preserve the order. I'll implement it in its own commit, we'll see what's preferable.
There was a problem hiding this comment.
Okay, I've rewritten the code now. I'm confident the SQL assignments do not care about the order fields are set in, so I've stuck with sets. I traced where the fields parameter can come from -- it looks like they are user-set (beet update -f FIELDS), in which case we really shouldn't be relying on the order anyway.
| return f"{m.group(2)} {m.group(1)}" | ||
|
|
||
| str1 = re.sub(SD_END_REPLACE, replacer, str1) | ||
| str2 = re.sub(SD_END_REPLACE, replacer, str2) | ||
|
|
There was a problem hiding this comment.
I disagree with @RollingStar actually, I think your regex is clearer. Maybe I'm just more familiar/confident with regex? One thing I would note is that it might be better, if we stay with this approach, to make the regex case-insensitive.
The logic is a bit easier to follow now. See: <beetbox#5337 (comment)>
Fixes #5293.
Along the way, I've made some small code improvements beyond just the use of f-strings; I don't think these will conflict with others' work. In particular, I've avoided modifying the formatting of paths; there is work to greatly simplify that using 'pathlib', at which point a second commit can clean them up.