Skip to content

Use lazy (old style %-format) log formatting in the new pylint config #1334

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

Closed
sechkova opened this issue Apr 6, 2021 · 6 comments · Fixed by #1383
Closed

Use lazy (old style %-format) log formatting in the new pylint config #1334

sechkova opened this issue Apr 6, 2021 · 6 comments · Fixed by #1383
Labels
discussion Discussions related to the design, implementation and operation of the project

Comments

@sechkova
Copy link
Contributor

sechkova commented Apr 6, 2021

Description of issue or feature request:
Using f-strings (and all types of string formatting except %-syle) in logging messages is not lazy evaluated so linters "recommend" %-style formatting.

Should we disable the related pylint warnings and use f-strings:

logging-not-lazy
logging-format-interpolation
logging-fstring-interpolation

or adhere to the recommendation.

Current behavior:
Top-level pylintrc disables logging that recommend lazy formatting globally.
New api/pylintrc does not.

Expected behavior:
Decide on a common approach.

@sechkova
Copy link
Contributor Author

sechkova commented Apr 6, 2021

An example approach to avoid the warnings while using f-strings which feels more like a workaround: 438c0a0

@sechkova sechkova added the discussion Discussions related to the design, implementation and operation of the project label Apr 6, 2021
@jku
Copy link
Member

jku commented Apr 7, 2021

It's annoying to use a different formatting method for logging than anything else but the reasoning is solid... Most of our logging is not in a "hot loop" but some debug logs really might be used quite a lot (and the workaround only hides the warning, not the potential performance issue in those cases).

I think using %-style is fine for logging: logger.debug("processing, a=%s", a) but it's not a strong opinion -- in the opposite case we may want to do a quick review to identify hottest logging calls though.

@sechkova sechkova changed the title Decide on lazy log lormatting in the new pylint config Decide on lazy log formatting in the new pylint config Apr 7, 2021
@sechkova
Copy link
Contributor Author

sechkova commented Apr 8, 2021

And then there is this issue when trying to use %-style formatting but you've set up pylint to logging-format-style=new.

@jku
Copy link
Member

jku commented Apr 9, 2021

I had understood that using %-style implies means we would need to use logging-format-style=old anyway (or rather not set it as that's the default)? Pylint docs really could have more examples.

sechkova added a commit to sechkova/tuf that referenced this issue Apr 14, 2021
Disable pylint's "Use lazy % formatting in logging functions"
warning until a common logging approach is decided. See theupdateframework#1334.

Signed-off-by: Teodora Sechkova <[email protected]>
@jku
Copy link
Member

jku commented May 6, 2021

I'm proposing we use the default logging style, meaning:

  • use logging-format-style=old (which is default value)
  • stop disabling logging-not-lazy (note: this is done only in some experimental client branches -- metadata api itself hasn't needed this because there's zero logging so far 😬 )

It looks a bit uglier (other string formatting is f-strings but logging is then based on %-formatting) but:

  • it's a common style
  • it's suggested by the google style guide
  • it means we don't have to think about performance when writing logging code

@joshuagl
Copy link
Member

Proposal above makes sense to me, if it's what's suggested by the style guide we probably don't even need to capture anything for this? Let's just do what our style guide suggests.

@jku jku changed the title Decide on lazy log formatting in the new pylint config Use lazy (old style %-format) log formatting in the new pylint config May 11, 2021
jku pushed a commit to jku/python-tuf that referenced this issue May 11, 2021
This is suggested by the Google style guide: the old style logging
(%-format) allows the log strings to be lazily formatted so there's less
need to think about performance when forming debug messages.

No actual code changes are needed because the metadata API does not yet
log anything.

Fixes theupdateframework#1334

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku closed this as completed in #1383 May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussions related to the design, implementation and operation of the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants