some pylint tidyups#502
Conversation
simplification of some conditional statements fixing of import orders cleaning up of unneeded chars
svlandeg
left a comment
svlandeg
left a comment
There was a problem hiding this comment.
Thanks for the PR! Definitely appreciate your contribution.
I left a few spurious comments. I think it will be up to tiangolo to decide whether we want some of these refactorings, or whether we'd keep to the original repo source style.
| }, | ||
| ) | ||
| if not (200 <= response.status_code <= 300): | ||
| if not 200 <= response.status_code <= 300: |
There was a problem hiding this comment.
When there's multiple if statements (implicit here because of the chaining) combined with a negation, I personally find keeping the parentheses more clear & readable - so I would vote to revert this change.
| if username in existing_usernames: | ||
| print("The user already exists") | ||
| raise typer.Exit() | ||
| else: | ||
| print(f"User created: {username}") | ||
| print(f"User created: {username}") |
There was a problem hiding this comment.
Throughout the docs, the house style is to use else even if the preceding if raises or returns. While I agree with you that this is not strictly necessary, I do think it contributes to readability to have the else explicitly there. Furthermore, it's more robust in terms of maintenance down the line: if the original if statement ever changes its implementation, the developer won't have to worry about implicit else statements being affected. So overall I think verbosity is prefered in these cases.
| def hello(count, name): | ||
| """Simple program that greets NAME for a total of COUNT times.""" | ||
| for x in range(count): | ||
| click.echo("Hello %s!" % name) | ||
| click.echo(f"Hello {name}!") |
There was a problem hiding this comment.
While F-strings are typically nicer and more readable, there are a few cases where you might prefer not using them. When we're logging things for instance, you might prefer avoiding the str() functionality for cases where the statement wouldn't actually get logged (e.g. if the logging level is higher). That said - I'm not actually sure how click.echo() operates under the hood so I'm not sure whether we want this rewrite in this case or not.
There was a problem hiding this comment.
Ah! This one is just that it was an exact copy of Click's tutorial. But they now updated it to use f-strings, so I would take this change. 🤓
|
Thanks @marksmayo! 🤓 And thanks @svlandeg for the thorough review! 🙇 For most of the code style changes I would just use what is configured in pre-commit (I'm not a fan of PyLInt, actually). Soon, after removing support for Python 3.6, I will migrate all to Ruff. This PR covers too many different things, so it would be better to split it to be able to take some things while not taking others. Specifically, I would take the changes in the Click examples from For now, I'll close this one, but feel free to add a new PR changing those Click f-strings! 🚀 |
3 participants
simplification of some conditional statements
fixing of import orders
cleaning up of unneeded chars