Modernize Python code #1769
Modernize Python code #1769
Conversation
Use f-strings, walrus operator and raise SystemExit instead of sys.exit. Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
| if sys.platform == "win32": | ||
| import subprocess | ||
|
|
||
| completed_process = subprocess.run([uv, *sys.argv[1:]], env=env) | ||
| sys.exit(completed_process.returncode) | ||
| else: | ||
| os.execvpe(uv, [uv, *sys.argv[1:]], env=env) | ||
| completed_process = subprocess.run(cmd, env=env) | ||
| raise SystemExit(completed_process.returncode) | ||
|
|
||
| os.execvpe(uv, cmd, env=env) |
There was a problem hiding this comment.
Hmm, I kinda prefer the if/else here -- I feel like it makes it more explicit that it's a binary choice: for Windows we do one thing; for all other platforms, we do a different thing
There was a problem hiding this comment.
But raise SystemExit makes it redundant, 🤔 so I prefer this 😆 the raise makes it explicit that the execution of that branch ends there.
There was a problem hiding this comment.
I agree with you that calling sys.exit is redundant, since all it does internally is raise SystemExit, so I too prefer raising SystemExit directly as a matter of style. But I'd still personally find it more readable, in this specific instance, to do this:
if sys.platform == "win32":
import subprocess
completed_process = subprocess.run(cmd, env=env)
raise SystemExit(completed_process.returncode)
else:
os.execvpe(uv, cmd, env=env)even though, as you say, the else is, strictly speaking, redundant.
Make a bikesheddy PR, expect bikesheddy review comments ;)
There was a problem hiding this comment.
I prefer my style, but if there's one more maintainer vote on the previous form can revert 👍
|
I think we're technically 3.7-compatible right now, so we may have to decide whether that's correct or not. |
https://github.com/astral-sh/uv/blob/main/pyproject.toml#L10 seems to strongly disagree 😆 any modern installer would refuse installing the package in 3.7 because of that. |
|
We should be as backwards compatible as possible here. I do not think we should make these cosmetic changes. |
|
Backwards compatible to what? Python 1.0? As pointed out above, the pyproject.toml metadata already makes you incompatible with anything before 3.8 for any installer that's not 5+ years old… |
|
We do not want to encourage use of Python 3.7, but we are compatible except for that line preventing installation with it. I think we should consider explicitly breaking runtime compatibility separately. Separate from the compatibility comments, I'm unwilling to make stylistic changes to such critical code unless accompanied by other necessary changes — the risk of breaking things for our users is not worth it. I hope this doesn't come across as harsh, this just isn't the time for us to be reviewing these kinds of changes. We're focused on fixing bugs and improving correctness to unblock new users. |
|
Fine, :-) I'll go away then. |
|
We do appreciate you! |
|
FYI just two days ago Pip dropped functional and runtime support for Python 3.7: pypa/pip#11944 The motivating factor there was vendoring other Python packages. Something uv doesn't have to worry about I guess. |
Use f-strings, walrus operator and raise SystemExit instead of sys.exit.
Signed-off-by: Bernát Gábor bgabor8@bloomberg.net