Add type annotations and drop Python 3.5 host support

[YannickJadoul]

Finally pushing this, after this came up again in #307. As noted, this is based on an old version of master, before a bunch of refactoring PRs were merged, so it still needs to be rebased (which might be as hard as just doing it from scratch again?).

To check, install mypy and run mypy --strict cibuildwheel/.

Mainly meant to open the discussion of which aspects of type annoations are useful in the context of cibuildwheel and which are probably not. Python's typing is not all or nothing, so we can surely find middle ground.

@joerick, @Czaki, I think you were both interested to see this? :-)

EDIT:
Closes #258

[Czaki]

Great job.
Looking on this code I think that installl_cpython and install_pypy should get as argument PythonConfiguration, not only selected fields. This will give better flexibility.

And when drop execution on python 3.5 they maybe it is a good idea to return Path object from them.

[YannickJadoul]

Great job.

Thanks! Thanks for the quick review as well :-)

Looking on this code I think that installl_cpython and install_pypy should get as argument PythonConfiguration, not only selected fields. This will give better flexibility.

I don't know if we need that flexibility currently, but I'll have a look after rebasing. Too many things have changed anyway.

And when drop execution on python 3.5 they maybe it is a good idea to return Path object from them.

There are lots of things to do when dropping 3.5 support. pathlib is not really related to types, I'd say (though types will help refactoring). For types, most importantly, there's var: Type = abc rather than var = abc # type: Type and the NamedTuple. I'll make an issue with a list!

EDIT: If there's more Python >= 3.6 stuff, do comment on #316.

[YannickJadoul]

There are still quite a few things I don't like, though:

• I don't mind importing typing for type annotations, but it's weird/ugly that it suddenly takes over the role of collections.namedtuple. typing is for types, not for functionality!?
• The @overloads of __main__.get_option_from_environment are exaggerated (I couldn't find another way to specify the return type correctly than with @overload). There's more code to define/type the function than in it's body. And flake8 acts annoying about style, as it wants me to put ... on the next line ánd have 2 lines of whitespace in between (so I added # noqa). Kind of defeats the purpose of a quick utility function?
• Importing ParsedEnvironment just for typing is quite annoying as well? Where's the nice duck-typing gone?

[Czaki]

There are still quite a few things I don't like, though:

• I don't mind importing typing for type annotations, but it's weird/ugly that it suddenly takes over the role of collections.namedtuple. typing is for types, not for functionality!?

For me best is dataclass, but it is introduced in python 3.7. Current option is only one which gives type support and do not force us to write own constructor.

• The @overloads of __main__.get_option_from_environment are exaggerated (I couldn't find another way to specify the return type correctly than with @overload). There's more code to define/type the function than in it's body. And flake8 acts annoying about style, as it wants me to put ... on the next line ánd have 2 lines of whitespace in between (so I added # noqa). Kind of defeats the purpose of a quick utility function?

maybe use pyi for some files (it need to move all typing to this file) to hide typing from main file.

• Importing ParsedEnvironment just for typing is quite annoying as well? Where's the nice duck-typing gone?

Something for something. You got type cheeking, but need to write more code.

You do not think about introduce type check task (as flake8) to tests?

[YannickJadoul]

You do not think about introduce type check task (as flake8) to tests?

Yes, I will. But figuring that out was not my main concern. Something to think about after rebase, and once we can make decisions.
That's why I just wrote the mypy --strict cibuildwheel/ command, for now.

[YannickJadoul]

Is there any PR I should wait for before rebasing? If not, I'll try to do it somewhere over the next few days. I've noticed it doesn't make a lot of sense to discuss code too much, if it might still change substantially.

[joerick]

So. In general, I'm happy with the way this is heading. Yes, a detailed review would have to wait until we have an up-to-date PR. Let's try to get a few of the open PRs in before doing that - I'm thinking about #295 and #311 mainly, but some of the smaller ones might as well land, too.

[joerick]

• I don't mind importing typing for type annotations, but it's weird/ugly that it suddenly takes over the role of collections.namedtuple. typing is for types, not for functionality!?

Strong agree. Always felt weird to me. Why didn't they just type-enable the version in collections? But it's not a big deal to me.

• The @overloads of __main__.get_option_from_environment are exaggerated (I couldn't find another way to specify the return type correctly than with @overload). There's more code to define/type the function than in it's body. And flake8 acts annoying about style, as it wants me to put ... on the next line ánd have 2 lines of whitespace in between (so I added # noqa). Kind of defeats the purpose of a quick utility function?

Yeah. I don't think there's anything we can do to mypy to improve this, but maybe we can tweak the flake8 rules to help here. Let's explore in the new/rebased PR?

• Importing ParsedEnvironment just for typing is quite annoying as well? Where's the nice duck-typing gone?

Will presumably be helped by the BuildOptions struct in #311.

[joerick]

Another note - I vote we drop Python 3.5 support in the same PR that we do the typing work. It will make things much easier. But let's leave the pathlib refactor for now - that will be much easier once we have types.

[YannickJadoul]

Side note, after all the complaining: on the bright side, running mypy and annotating and fixing these issues is a lot of fun and satisfaction as well ;-) (I just wish there'd be less clutter remaining in the code afterwards.)

[YannickJadoul]

First rebase. Got curious, and it actually went a lot quicker than expected. Juts a few extra things to annotate, now.

[Czaki]

Progress?

[YannickJadoul]

Progress?

On what? I think there aren't a lot of open discussions, for the moment? But this is not for the upcoming release, yet, if I understood @joerick correctly.

Seems recent PRs resulted in conflicts, though. I'll rebase, in a bit!

[Czaki]

@YannickJadoul I do not want to create next PR until types are finished/decided.

[YannickJadoul]

Rebased

[Czaki]

There is backport dataclasses module to python 3.6 https://pypi.org/project/dataclasses/

[YannickJadoul]

There is backport dataclasses module to python 3.6 https://pypi.org/project/dataclasses/

Is that worth it? Not a big advantage over NamedTuple, I thought?

[Czaki]

dataclass allow to create modifiable container.
It comes outside typing module:

I don't mind importing typing for type annotations, but it's weird/ugly that it suddenly takes over the role of collections.namedtuple. typing is for types, not for functionality!?

It also have few nice features like dataclasses.field

[YannickJadoul]

dataclass allow to create modifiable container.

But if we don't need mutable containers, it might be better to not do this?

It comes outside typing module:

I don't mind importing typing for type annotations, but it's weird/ugly that it suddenly takes over the role of collections.namedtuple. typing is for types, not for functionality!?

It also have few nice features like dataclasses.field

OK, yes, agreed there's some nice things, definitely. But not sure it's worth adding the extra dependency to the core of cibuildwheel? :-/

[Czaki]

This is dependency only for python 3.6. From Python 3.7 it is built-in module.

The question is If nice features of dataclass will be useful. If Yes then I not think it is good to wait with this till python 3.6 is dropped if it can be introduced earlier.

[joerick]

I'm not so interested in the dataclass - config should be an immutable object anyway.

I think we should try to get this in soon, before #334, if that works for you @YannickJadoul! I'll take another look over now, but I think it's pretty close, it probably just needs a merge of upstream master.

[YannickJadoul]

Alright. I'll see if I can fit in some time to rebase today or tomorrow. Still want to/need to find some time to decently review #334 as well.

[joerick]

Just a few notes. Then we'd need:

• some mypy config somewhere (equivalent to --strict), either in setup.cfg or in mypy.ini
• Type checking in CI. I guess either this is an explicit job, or maybe this would be convenient to roll it into flake8 https://pypi.org/project/flake8-mypy/

[YannickJadoul]

Just a few notes. Then we'd need:

Thanks! Rebased, solved the conflicts, and fixed the final few things

• some mypy config somewhere (equivalent to --strict), either in setup.cfg or in mypy.ini

Done. Added to setup.cfg since that already contains flake8 config anyway. Or any reason to prefer mypy.ini?

There's a bunch of other flags/config values to be set, with various more errors/warnings. Nothing immediately catches my eye, but might be interesting to go over? Depends a bit on how strict you want to be; e.g. disallowing Any could be interesting in some places, to ensure annotations will be added in new code?

• Type checking in CI. I guess either this is an explicit job, or maybe this would be convenient to roll it into flake8 https://pypi.org/project/flake8-mypy/

I was planning to just install mypy and run it in the same job (but different call) as flake8. Do you prefer flake8-mypy? To be fair, it's neat that it can be integrated, but I don't really see the advantage to add it as extra dependency (but it should be a minor/stable dependency, ofc).

[joerick]

Wonderful. Merge when you're ready. I'll let you hit the green button :)

[YannickJadoul]

Hèhè, thanks! :-)