Re: [BuildStream] Proposal: Consider adding type hints



Hi all,

Thanks for the quick (and positive) responses.

It seems like we are generally in agreement that this would be a good idea. So,
I will slowly start work in that direction, and create appropriate
issues/merge requests on GitLab.

Some responses below.

I do think this change should definitely come with the addition to CI of
a choice of one of these - it would be hard to spot its usefulness without
them.

I mostly agree here. We can start with one of these analyzers, probably in a
less strict mode to begin with. In some other projects, we have started
using mypy and have had generally good experience with it. So, unless anyone
has any strong preferences, I'd start with that.

This brings up an interesting point; currently our public API does not
specify what container types like dict or list should contain. Python
actually supports hints like Dict[str, str].

While, to my knowledge, we technically only accept specific types in those
containers, changing that breaks that contract (since someone could have
written plugins with types that quack like ducks, but don't inherit from
ducks). Yet I feel that adding more specific hints would make this more
useful.

Do we want to become more strict about types in our API surface as part
of this change, given that we technically already are that strict?

This is a very valid question. I generally prefer to have more specific type
hints, wherever possible. So, I'd be in favor of adding hints like
Dict[str, str].

One thing to remember though is that these type hints are just that - hints for
static analyzers. They shouldn't stop any plugin from working (unless we also
add strict runtime checks). So, I am not sure if this is really breaking any
contract. In any case, since we are breaking API with BuildStream 2.0 anyway, I
don't think this should be a big issue.

Just bear in mind that we support >= python3.5, it seems that variable
annotations [8] e.g. `pi: float = 3.14159` were not introduced until
Python 3.6.

Just chiming in here on that limitation... given that BuildStream 2 is
not going to be released for quite some time, it may be fair to drop
python 3.5 support if it becomes unwieldy, at least something that we
can consider.

Good to hear that it's an option. However, to be honest, I don't think we need
to drop support for Python 3.5 just to be able to use this syntax. Especailly,
given that there are workarounds for this use-case. But we can reconsider this
if the alternatives also turn out to be too unwieldy.


- Chandan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]