-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Replace most str.format()
uses with f-strings
#5337
base: master
Are you sure you want to change the base?
Changes from all commits
3ab272d
5496060
c3682e7
9575fc9
3a61d49
6290b0b
79a3495
4acbf63
7dc6a97
fcf9a78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,10 +396,9 @@ def _awaken( | |
return obj | ||
|
||
def __repr__(self) -> str: | ||
return "{}({})".format( | ||
type(self).__name__, | ||
", ".join(f"{k}={v!r}" for k, v in dict(self).items()), | ||
) | ||
name = type(self).__name__ | ||
fields = ", ".join(f"{k}={repr(v)}" for k, v in dict(self).items()) | ||
return f"{name}({fields})" | ||
|
||
def clear_dirty(self): | ||
"""Mark all fields as *clean* (i.e., not needing to be stored to | ||
|
@@ -414,10 +413,11 @@ def _check_db(self, need_id: bool = True) -> Database: | |
has a reference to a database (`_db`) and an id. A ValueError | ||
exception is raised otherwise. | ||
""" | ||
name = type(self).__name__ | ||
if not self._db: | ||
raise ValueError("{} has no database".format(type(self).__name__)) | ||
raise ValueError(f"{name} has no database") | ||
if need_id and not self.id: | ||
raise ValueError("{} has no id".format(type(self).__name__)) | ||
raise ValueError(f"{name} has no id") | ||
|
||
return self._db | ||
|
||
|
@@ -558,12 +558,12 @@ def __iter__(self) -> Iterator[str]: | |
|
||
def __getattr__(self, key): | ||
if key.startswith("_"): | ||
raise AttributeError(f"model has no attribute {key!r}") | ||
raise AttributeError(f"model has no attribute {repr(key)}") | ||
else: | ||
try: | ||
return self[key] | ||
except KeyError: | ||
raise AttributeError(f"no such field {key!r}") | ||
raise AttributeError(f"no such field {repr(key)}") | ||
|
||
def __setattr__(self, key, value): | ||
if key.startswith("_"): | ||
|
@@ -580,46 +580,53 @@ def __delattr__(self, key): | |
# Database interaction (CRUD methods). | ||
|
||
def store(self, fields: Optional[Iterable[str]] = None): | ||
"""Save the object's metadata into the library database. | ||
:param fields: the fields to be stored. If not specified, all fields | ||
will be. | ||
""" | ||
if fields is None: | ||
fields = self._fields | ||
Save the object's metadata into the library database. | ||
|
||
:param fields: the fields to be stored (default: all of them). | ||
Only non-flexible fields (excluding `"id"`) can be specified. | ||
""" | ||
|
||
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) | ||
# Extract known sets of keys from the table. | ||
known_fields = set(self._fields.keys()) | ||
known_flex_fields = set(self._values_flex.keys()) | ||
|
||
# Normalize the 'fields' parameter. | ||
fields = set(fields or known_fields) - {"id"} | ||
assert ( | ||
len(fields & known_flex_fields) == 0 | ||
), "`fields` cannot contain flexible fields" | ||
|
||
# Compute how various fields were modified. | ||
dirty_fields = fields & self._dirty | ||
dirty_flex_fields = known_flex_fields & self._dirty | ||
removed_flex_fields = self._dirty - fields - known_flex_fields | ||
|
||
with db.transaction() as tx: | ||
# Main table update. | ||
if assignments: | ||
query = "UPDATE {} SET {} WHERE id=?".format( | ||
self._table, ",".join(assignments) | ||
) | ||
subvars.append(self.id) | ||
tx.mutate(query, subvars) | ||
# Update non-flexible fields. | ||
if dirty_fields: | ||
# NOTE: the order of iteration of 'dirty_fields' will not change | ||
# between the two statements, since the set is not modified. | ||
assignments = ",".join(f"{k}=?" for k in dirty_fields) | ||
values = [self._type(k).to_sql(self[k]) for k in dirty_fields] | ||
query = f"UPDATE {self._table} SET {assignments} WHERE id=?" | ||
tx.mutate(query, [*values, self.id]) | ||
|
||
# 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), | ||
) | ||
# TODO: Use the underlying 'executemany()' function here. | ||
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]), | ||
) | ||
|
||
# Deleted flexible attributes. | ||
for key in self._dirty: | ||
# TODO: Use the underlying 'executemany()' function here. | ||
for key in removed_flex_fields: | ||
tx.mutate( | ||
f"DELETE FROM {self._flex_table} WHERE entity_id=? AND key=?", | ||
(self.id, key), | ||
|
@@ -1192,18 +1199,17 @@ def _make_table(self, table: str, fields: Mapping[str, types.Type]): | |
columns = [] | ||
for name, typ in fields.items(): | ||
columns.append(f"{name} {typ.sql}") | ||
setup_sql = "CREATE TABLE {} ({});\n".format( | ||
table, ", ".join(columns) | ||
) | ||
columns_def = ", ".join(columns) | ||
setup_sql = f"CREATE TABLE {table} ({columns_def});\n" | ||
|
||
else: | ||
# Table exists does not match the field set. | ||
setup_sql = "" | ||
for name, typ in fields.items(): | ||
if name in current_fields: | ||
continue | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flagging here that we always want to be attentive when changing SQL commands. |
||
|
||
with self.transaction() as tx: | ||
|
@@ -1215,16 +1221,16 @@ def _make_attribute_table(self, flex_table: str): | |
""" | ||
with self.transaction() as tx: | ||
tx.script( | ||
""" | ||
CREATE TABLE IF NOT EXISTS {0} ( | ||
f""" | ||
CREATE TABLE IF NOT EXISTS {flex_table} ( | ||
id INTEGER PRIMARY KEY, | ||
entity_id INTEGER, | ||
key TEXT, | ||
value TEXT, | ||
UNIQUE(entity_id, key) ON CONFLICT REPLACE); | ||
CREATE INDEX IF NOT EXISTS {0}_by_entity | ||
ON {0} (entity_id); | ||
""".format(flex_table) | ||
CREATE INDEX IF NOT EXISTS {flex_table}_by_entity | ||
ON {flex_table} (entity_id); | ||
""" | ||
) | ||
|
||
# Querying. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,6 +151,7 @@ | |
self.fast = fast | ||
|
||
def col_clause(self) -> Tuple[str, Sequence[SQLiteType]]: | ||
# TODO: Avoid having to insert raw text into SQL clauses. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good edit |
||
return self.field, () | ||
|
||
def clause(self) -> Tuple[Optional[str], Sequence[SQLiteType]]: | ||
|
@@ -170,7 +171,7 @@ | |
|
||
def __repr__(self) -> str: | ||
return ( | ||
f"{self.__class__.__name__}({self.field_name!r}, {self.pattern!r}, " | ||
f"{self.__class__.__name__}({repr(self.field_name)}, {repr(self.pattern)}, " | ||
f"fast={self.fast})" | ||
) | ||
|
||
|
@@ -209,7 +210,9 @@ | |
return obj.get(self.field_name) is None | ||
|
||
def __repr__(self) -> str: | ||
return f"{self.__class__.__name__}({self.field_name!r}, {self.fast})" | ||
return ( | ||
f"{self.__class__.__name__}({repr(self.field_name)}, {self.fast})" | ||
) | ||
|
||
|
||
class StringFieldQuery(FieldQuery[P]): | ||
|
@@ -303,7 +306,7 @@ | |
return unicodedata.normalize("NFC", s) | ||
|
||
@classmethod | ||
def string_match(cls, pattern: Pattern, value: str) -> bool: | ||
return pattern.search(cls._normalize(value)) is not None | ||
|
||
|
||
|
@@ -465,7 +468,7 @@ | |
"""Return a set with field names that this query operates on.""" | ||
return reduce(or_, (sq.field_names for sq in self.subqueries)) | ||
|
||
def __init__(self, subqueries: Sequence = ()): | ||
self.subqueries = subqueries | ||
|
||
# Act like a sequence. | ||
|
@@ -476,7 +479,7 @@ | |
def __getitem__(self, key): | ||
return self.subqueries[key] | ||
|
||
def __iter__(self) -> Iterator: | ||
return iter(self.subqueries) | ||
|
||
def __contains__(self, subq) -> bool: | ||
|
@@ -502,7 +505,7 @@ | |
return clause, subvals | ||
|
||
def __repr__(self) -> str: | ||
return f"{self.__class__.__name__}({self.subqueries!r})" | ||
return f"{self.__class__.__name__}({repr(self.subqueries)})" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snejus My reading of the PEP for f-strings led me to believe the opposite? They claim that they support the old calls, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good shout @Serene-Arc. What I gathered from that section is that they aren't required anymore since they can be replaced by the equivalent expressions, which was not supported previously. Regarding the preference, from what I've seen in the wild Python community tends to use the native syntax supported by the f-string instead of an equivalent expression, if possible, for example:
dt = datetime.now()
f"Datetime: {dt:%F %T}"
# vs
f'Datetime: {dt.strftime("%F %T")}'
txt = "text"
f"{txt:>10}"
# vs
f'{" " * (10 - len(txt))}{txt}' In a similar way, I prefer I asked perplexity to see what it thinks: https://www.perplexity.ai/search/should-r-or-repr-call-be-prefe-YGZ43OrrTGOYR2eNjl7QeQ |
||
|
||
def __eq__(self, other) -> bool: | ||
return super().__eq__(other) and self.subqueries == other.subqueries | ||
|
@@ -525,7 +528,7 @@ | |
"""Return a set with field names that this query operates on.""" | ||
return set(self.fields) | ||
|
||
def __init__(self, pattern, fields, cls: Type[FieldQuery]): | ||
self.pattern = pattern | ||
self.fields = fields | ||
self.query_class = cls | ||
|
@@ -547,7 +550,7 @@ | |
|
||
def __repr__(self) -> str: | ||
return ( | ||
f"{self.__class__.__name__}({self.pattern!r}, {self.fields!r}, " | ||
f"{self.__class__.__name__}({repr(self.pattern)}, {repr(self.fields)}, " | ||
f"{self.query_class.__name__})" | ||
) | ||
|
||
|
@@ -563,7 +566,7 @@ | |
query is initialized. | ||
""" | ||
|
||
subqueries: MutableSequence | ||
|
||
def __setitem__(self, key, value): | ||
self.subqueries[key] = value | ||
|
@@ -618,7 +621,7 @@ | |
return not self.subquery.match(obj) | ||
|
||
def __repr__(self) -> str: | ||
return f"{self.__class__.__name__}({self.subquery!r})" | ||
return f"{self.__class__.__name__}({repr(self.subquery)})" | ||
|
||
def __eq__(self, other) -> bool: | ||
return super().__eq__(other) and self.subquery == other.subquery | ||
|
@@ -791,9 +794,7 @@ | |
|
||
def __init__(self, start: Optional[datetime], end: Optional[datetime]): | ||
if start is not None and end is not None and not start < end: | ||
raise ValueError( | ||
"start date {} is not before end date {}".format(start, end) | ||
) | ||
raise ValueError(f"start date {start} is not before end date {end}") | ||
self.start = start | ||
self.end = end | ||
|
||
|
@@ -841,20 +842,18 @@ | |
date = datetime.fromtimestamp(timestamp) | ||
return self.interval.contains(date) | ||
|
||
_clause_tmpl = "{0} {1} ?" | ||
|
||
def col_clause(self) -> Tuple[str, Sequence[SQLiteType]]: | ||
clause_parts = [] | ||
subvals = [] | ||
|
||
# Convert the `datetime` objects to an integer number of seconds since | ||
# the (local) Unix epoch using `datetime.timestamp()`. | ||
if self.interval.start: | ||
clause_parts.append(self._clause_tmpl.format(self.field, ">=")) | ||
clause_parts.append(f"{self.field} >= ?") | ||
subvals.append(int(self.interval.start.timestamp())) | ||
|
||
if self.interval.end: | ||
clause_parts.append(self._clause_tmpl.format(self.field, "<")) | ||
clause_parts.append(f"{self.field} < ?") | ||
subvals.append(int(self.interval.end.timestamp())) | ||
|
||
if clause_parts: | ||
|
@@ -908,7 +907,7 @@ | |
""" | ||
return None | ||
|
||
def sort(self, items: List) -> List: | ||
"""Sort the list of objects and return a list.""" | ||
return sorted(items) | ||
|
||
|
@@ -978,7 +977,7 @@ | |
return items | ||
|
||
def __repr__(self): | ||
return f"{self.__class__.__name__}({self.sorts!r})" | ||
return f"{self.__class__.__name__}({repr(self.sorts)})" | ||
|
||
def __hash__(self): | ||
return hash(tuple(self.sorts)) | ||
|
@@ -1018,7 +1017,7 @@ | |
def __repr__(self) -> str: | ||
return ( | ||
f"{self.__class__.__name__}" | ||
f"({self.field!r}, ascending={self.ascending!r})" | ||
f"({repr(self.field)}, ascending={repr(self.ascending)})" | ||
) | ||
|
||
def __hash__(self) -> int: | ||
|
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.
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 changereplacer
to something that sounds more like a function? Like do_replace?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.
That's fair. I think a more intuitive implementation would be based on
str.rsplit()
orstr.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
andreplacer
->move_article_to_front
?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.
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.
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.
I've rewritten this as a much more explicit and intuitive function which uses
str.rsplit
to separate a title from an article. @RollingStar, I hope this makes it more readable! Also, @Serene-Arc, the inputs are lower-cased here, so case sensitivity is not an issue.