Pack build arguments #311
Pack build arguments #311
Conversation
cibuildwheel/util.py
Outdated
| BuildOptions = NamedTuple("BuildOptions", [ | ||
| ("project_dir", str), | ||
| ("output_dir", str), | ||
| ("test_command", str), |
There was a problem hiding this comment.
yes. I think it is empty string when undefined. Or maybe it should be setted to default as empty sting (in main)?
| @@ -103,5 +107,39 @@ def get_for_python_version(self, version): | |||
| return self.base_file_path | |||
|
|
|||
|
|
|||
| BuildOptions = NamedTuple("BuildOptions", [ | |||
There was a problem hiding this comment.
Ew, I didn't know 3.5 only had this style of NamedTuple...
You know, I've been considering dropping Python 3.5 support on the host side - 3.6 has so many great features (proper pathlib.Path support, f-strings, better typing)...
There was a problem hiding this comment.
Strongly agree. It may also reduce test time.
There was a problem hiding this comment.
Are the types important?
Maybe we should keep the types for a separate PR? I have some changes stashed away that are probably completely outdated after all the refactoring, but I could bring try to rebase them. And types are pretty useless without tooling to check them, anyway (as demonstrated by the tests passing without the correct Optional[str]s)?
At any rate, these were the types I found
def build(project_dir: str, output_dir: str, build_selector: Callable[[str], bool],
manylinux_images: Dict[str, str], environment: ParsedEnvironment,
before_build: Optional[str], build_verbosity: int, repair_command: str,
test_command: Optional[str], test_requires: List[str], test_extras: str) -> None:
There was a problem hiding this comment.
Are the types important?
Maybe we should keep the types for a separate PR?
I do not see big difference between current NamedTuple and untyped one.
Removing python 3.5 should be in separate PR.
And types are pretty useless without tooling to check them, anyway (as demonstrated by the tests passing without the correct Optional[str]s
Even vscode have basic type support and provide hinting base on them. Also marking type mismatch is avaliable.
There was a problem hiding this comment.
Even vscode have basic type support and provide hinting base on them. Also marking type mismatch is avaliable.
We didn't have that before, either, did we? That's why I'm wondering if that shouldn't be discussed in the context of #258 and a related PR.
There was a problem hiding this comment.
I'm increasingly open to typing, having had better experiences with it through the VS Code Python extension (you need to disable Jedi in their settings, but it's fantastic).
Typing offers three benefits as I see it
- inline documentation of types (for others reading the source code)
- metadata for editors to provide auto-complete
- type checking, to ensure the above is accurate, and to find bugs before the code is run
So for the sake of the first two, I'm happy for this to be a typing.NamedTuple. It's feeling increasingly likely that types will eventually come to the project, so hopefully we'll be able to check them soon, too.
@YannickJadoul you've mentioned in the past that you had played around with project-level types - I'd be really curious to see how that looks if you're able to share the branch?
There was a problem hiding this comment.
OK, fair enough. I just feels weird to introduce typing in this PR.
@joerick I'll try to push what I had later today, even though it'll be outdated.
There was a problem hiding this comment.
But this PR introduces types only in new part. Does not type existing one.
cibuildwheel/util.py
Outdated
| ("test_command", str), | ||
| ("test_requires", List[str]), | ||
| ("test_extras", str), | ||
| ("before_build", str), |
|
How comes tests here passed (I assume), but |
|
Also, it's really a bad one:
So why did the CI let this pass? Really weird. |
|
Ah! It's that problem where CircleCI doesn't run on Czaki's PRs. Would you mind investigating/contacting them about it @Czaki? It's only your PRs that don't run, so I suspect it's something to do with your account |
|
Argh. I thought I'd fixed it in 25f2e8c, but then I'd only pushed it to my repo, and not to |
When I prepare next PR I will try to find the reason. It may be connected with setup of circleCI on my account. I disable it for cibuildwheel So maybe in next PR it will work. |
This is PR with packing all argument of build function into single structure. This is extreme version of shortening it signature. I'm not sure if it is best solution, but also partially show how it look if there is try of introduce inheritance.
I mark it as draft to ask you about opinion. Maybe it is better to create few structures and pack argument in few groups (like setup, test, ...).