-
Notifications
You must be signed in to change notification settings - Fork 8
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
chore: do not use f-strings in logging statements #32
chore: do not use f-strings in logging statements #32
Conversation
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.
Thanks for doing this chore @agriyakhetarpal!
Thanks for reviewing it! |
@@ -0,0 +1,2 @@ | |||
# Enable Ruff G004 to enforce logging best practices | |||
4c3043200ca15ade613fa3e3f8ca84bbe3522f21 |
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.
What's this file 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.
When we make formatting/style changes in the code that are not useful for the git blame
, we can ignore the commits (revisions) in which the changes were made in the relevant files to keep their history clean.
More on this here: https://graphite.dev/guides/git-blame-ignore-revs
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 that I fixed it – if you check any of the files, say, main
/pyodide_build/buildall.py (blame), it wouldn't show the formatting change in the history (by default).
Also, oops, I was using GitHub through my phone and I squash merged it instead of creating a merge commit 🤦🏻 Let me fix this in a new PR. |
This reverts commit b1dde9a.
Fixed this in the |
Description
Thanks to the tip in #30 (comment), I enabled a Ruff rule to avoid using f-strings in future logging statements we add: https://docs.astral.sh/ruff/rules/logging-f-string/, and converted the previous ones to comply with the rule. It's not necessarily required, but it's good to enable best practices.
Important
This PR should be merged with a merge commit, not squashed.