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

Do not use f-strings in log calls. #131

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Aug 26, 2024

It is in general bad practice; in particular if one want a structured logging handler that store the template string and the values separately, using f-strings makes it not possible.

I believe there is also a question of actually calling the formatter on the interpolated values which is not done if the loging level is low enough.

While it might not be too critical for micropip, I belive it is good practive to show the example.

It is in general bad practice; in particular if one want a structured
logging handler that store the template string and the values separately, using
f-strings makes it not possible.

I belive there is also a question of actually calling the formatter on
the interpolated values which is not done if the loging level is low
enough.

WHile it might not be too critical for micropip, I belive it is good
practive to show the example.
@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

I guess in a couple of places, %r, is better suited than '%s', but I didn't want to be too invasive with two semantically different changes that might make review difficult.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

See among other all the https://docs.astral.sh/ruff/rules/#flake8-logging-format-g

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

Actually, if there is a ruff lint, why not add it.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Carreau! I added all those logging stuff, and to be honest, I don't know much about the good logging practice. So please feel free to make suggestions if you found any issues with logging!

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

I think this is perfectly fine for a project like micropip, but I like to show good practice.

Just think that for really huge webapp, like facebook level, you want to store the template string and the values separately (potentially you store the template string only once), and not ins a log file but with mysql with 1 column per value. So that you can ask "show me all the identical messages", and/or if one of the value is say a float like in
log("page took %s ms to render", time), you can ask for mean/std.

And with that all those "don't use f-string" in logs will make sens.

I don't personally do use those kind of infra, but just knowing the above made it make sens to me.

@ryanking13
Copy link
Member

The failing test isn't related to this PR (timeout in the chrome browser). Let me go ahead and merge.

@ryanking13 ryanking13 merged commit 805140f into pyodide:main Aug 26, 2024
6 of 7 checks passed
@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

The failing test isn't related to this PR (timeout in the chrome browser). Let me go ahead and merge.

Thanks, I keep seeing those timeout, and don't know what to do about those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants