-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Pre-commit autoupdate #16462
Pre-commit autoupdate #16462
Conversation
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
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 think this is okay but then again, this is also io.fits
, better see if @saimn or @astrofrog are okay with this first.
@@ -1069,10 +1069,10 @@ def update_from_dict(k, v): | |||
card = Card(*((k,) + v)) | |||
else: | |||
raise ValueError( | |||
"Header update value for key %r is invalid; the " | |||
f"Header update value for key {k!r} is invalid; the " |
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.
Just curious, why is this not just {k}
?
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.
!r
calls repr
instead of str
, which is sometimes desirable and in particular will put strings in quotes. Here it's a strictly equivalent refactor.
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'm happy to change it, I was just keeping this strictly translated. But yes, {k}
is equivalent.
IMO I don't sweat these (unless for line length) and I'm just waiting for ruff
to eventually decide in one direction or another and auto fix everything with a pre-commit autoupdate. IDK why they haven't yet. A quick google found no discussion of this. Perhaps I'm missing a subtlety where f"{k}"
and f"{k!r}"
aren't actually equivalent. https://docs.astral.sh/ruff/rules/explicit-f-string-type-conversion/#why-is-this-bad talks about custom __format__
, so maybe?
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.
Perhaps I'm missing a subtlety where
f"{k}"
andf"{k!r}"
aren't actually equivalent.
They only appear equivalent for types that implement either __str__
or __repr__
but not both (because they default to each other). They are not the same in the str
type.
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.
Now I am confused. Do we change to {k}
or not? Reading https://stackoverflow.com/questions/33097143/what-is-the-difference-between-r-and-r-in-python just makes me even more confused.
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.
Have you looked up any explicit examples?
>>> greeting = "Hello!"
>>> print(f"{greeting}")
Hello!
>>> print(f"{greeting!r}")
'Hello!'
UPDATE: and the equivalent example using the old-style string formatting:
>>> greeting = "Hello!"
>>> print("%s" % greeting)
Hello!
>>> print("%r" % greeting)
'Hello!'
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.
This PR is meant to refactor code, not change behavior, so we should stick with{k!r}
.
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.
OK then, let's merge. Thanks, all!
Following #16396
Note that autoupdate_commit_msg doesn't appear to have a way to set the message body. Normally in git this can be done with a second
-m
or a-a
.