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

ngclient: Use %-style formatting in logging #1400

Closed
jku opened this issue May 20, 2021 · 4 comments · Fixed by #1495
Closed

ngclient: Use %-style formatting in logging #1400

jku opened this issue May 20, 2021 · 4 comments · Fixed by #1495
Assignees
Labels
good first issue Bite-sized items for first time contributors ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented May 20, 2021

(this is about code currently in experimental-client/ngclient branch, hopefully soon in develop)

We currently have some f-strings in logging:

                        msg = (
                            f"Adding child role {child_role_name}.\n",
                            "Not backtracking to other roles.",
                        )
                        logger.debug(msg)

Let's use %-style instead in logging (even if f-strings are great for other string formatting, see the google python style guide)

@joshuagl joshuagl added the good first issue Bite-sized items for first time contributors label May 20, 2021
jku pushed a commit to jku/python-tuf that referenced this issue May 21, 2021
Remove pylint disable logging-no-lazy, fix remaining non-lazy logging
(ngclient/updater.py still contains some but pylint does not notice
them: These will be fixed in issue theupdateframework#1400)

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku jku changed the title experimental client: Use %-style formatting in logging ngclient: Use %-style formatting in logging May 25, 2021
@sechkova sechkova added the backlog Issues to address with priority for current development goals label May 26, 2021
@sechkova sechkova added this to the weeks24-25 milestone Jun 9, 2021
@avelichka
Copy link
Contributor

I'd like to give it a try, please?

@trishankatdatadog
Copy link
Member

Curious: why does Google recommend % over f-strings? I think the latter is far more readable, no?

@jku
Copy link
Member Author

jku commented Jun 10, 2021

I agree, it's just that f-strings get greedily evaluated before logger can check if they are needed: with %-style the logger can check if the string is needed (and usually it isn't) before building the string. Obviously 99% of our logging is nowhere near a critical path where this would matter ... but I'd still like to be able to log inside e.g. a download loop without thinking about it.

The other reason would be that our linters complain :) We could of course modify our linter config (and document the style guide difference) but that feels like work too...

@trishankatdatadog
Copy link
Member

My 0.02 BTC is that we should keep developers not Google/linters/speedtests happy, but that's just me. I will let you guys decide 🙂

@sechkova sechkova modified the milestones: weeks24-25, weeks26-27 Jun 23, 2021
@joshuagl joshuagl modified the milestones: weeks26-27, Sprint 4 Jul 7, 2021
@joshuagl joshuagl removed the backlog Issues to address with priority for current development goals label Jul 7, 2021
@jku jku closed this as completed in #1495 Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Bite-sized items for first time contributors ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants