Skip to content
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

Merged
merged 2 commits into from
May 20, 2024
Merged

Pre-commit autoupdate #16462

merged 2 commits into from
May 20, 2024

Conversation

nstarman
Copy link
Member

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.

Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
@nstarman nstarman changed the title Pre commit Pre-commit May 15, 2024
@nstarman nstarman requested a review from pllim May 15, 2024 20:08
Copy link

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@nstarman nstarman changed the title Pre-commit Pre-commit autoupdate May 15, 2024
@nstarman nstarman added this to the v7.0.0 milestone May 15, 2024
Copy link
Member

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 "
Copy link
Member

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}?

Copy link
Contributor

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.

Copy link
Member Author

@nstarman nstarman May 19, 2024

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?

Copy link
Contributor

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}" and f"{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.

Copy link
Member

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.

Copy link
Member

@eerovaher eerovaher May 20, 2024

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!'

Copy link
Contributor

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}.

Copy link
Member

@pllim pllim left a 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!

@pllim pllim merged commit 8a38df7 into astropy:main May 20, 2024
28 of 30 checks passed
@nstarman nstarman deleted the pre-commit branch May 20, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants