chore: cleanup types #1263
chore: cleanup types #1263
Conversation
There was a problem hiding this comment.
I like the stronger Final types - I have also been enduring PyLance errors for these!
But I'm kinda -0 on most of the raise SystemExit changes. I've watched the video and I get it, but it just seems a little 'clever' to me. Importing sys isn't bad, and it can be useful information - the absence of a sys import is actually quite a useful signal to me that a Python file doesn't interact with the system and is more likely to be pure.
I'm -1 on uses of raise SystemExit(0), as these look like errors and that isn't how they're intended to be read.
bin/sample_build.py
Outdated
| raise SystemExit( | ||
| subprocess.run([sys.executable, "-m", "cibuildwheel"], cwd=project_dir).returncode | ||
| ) |
There was a problem hiding this comment.
This is poor form, IMO. I watched the video and follow the logic, but sys.exit is more idiomatic to me. The raise implies an error. I realise that the exception is how Python works internally, but I believe that it does that to conveniently reuse the try/finally semantics, and not because it should be viewed as an error. It comes across strangely in the case that this is raise SystemExit(0) - there, the program is exiting successfully but our code is raising an exception.
More familiar is a function that never returns, like C's exit(). That's also how most (all?) other mainstream languages do it, e.g. Rust, Ruby, Node.js, Java, Go...
cibuildwheel/__main__.py
Outdated
| @@ -211,7 +211,7 @@ def build_in_directory(args: CommandLineArguments) -> None: | |||
| if args.print_build_identifiers: | |||
| for identifier in identifiers: | |||
| print(identifier) | |||
| sys.exit(0) | |||
| raise SystemExit(0) | |||
There was a problem hiding this comment.
Also not super fond of this...
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii
force-pushed
the
henryiii/chore/cleanup
branch
from
26c1d76
to
f3df26d
Compare
PyLance doesn't like raw Final (and Tuples hard-code the length if you don't specify it).
Moved to #1270 to unblock this:
Also moved to raising SystemExit over sys.exit. Doesn't require an import & is just the implementation of
sys.exitanyway. See python: raise SystemExit (beginner - intermediate).