Skip to content

Concidering the use of type annotations in the code base #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ShaharNaveh opened this issue Jan 30, 2020 · 11 comments · Fixed by #315
Closed

Concidering the use of type annotations in the code base #258

ShaharNaveh opened this issue Jan 30, 2020 · 11 comments · Fixed by #315

Comments

@ShaharNaveh
Copy link

Type annotations (see pep 484) can be really powerful, and now when the code base is relatively small, IMO it's worth considering using it.

This is of course deserves a discussion (just my opinion)

@Czaki
Copy link
Contributor

Czaki commented Feb 2, 2020

Just 2 days ago in #245 there is decision to drop support execution of cibuildwheel under python 2.7. I think that adding type annotation should be part of migration.

@YannickJadoul
Copy link
Member

@MomIsBestFriend @Czaki Could you explain the advantage? I don't see the point of adding and maintaining type annotations without any good reason.

@Czaki
Copy link
Contributor

Czaki commented Feb 2, 2020

Better support for statistical analysis of code. Some of IDE inform that you use wrong type. Better information fore someone who would like to participate.

@YannickJadoul
Copy link
Member

I like typing, but I'm not convinced about the advantages it would bring to cibuildwheel, in the limited amount of code we have? But I was also kind of avoiding answering here, and just hoping @joerick would say "yes"/"no" :D

@ShaharNaveh
Copy link
Author

ShaharNaveh commented Feb 2, 2020

@YannickJadoul I mean, why not? there are a lot of reasons why to use static types, I will name just a few.

  • Better integration with IDE environments (as mentioned earlier), when I have static types for example in Pycharm it will give me much better suggestions for method, that are actually related to the type of my variable.

  • Has (small) speed boost

  • Better code readability, lemme give you an example.

Consider this piece of code:

def func(lst):
    for thing in lst:
        validate(thing)

Clearly you cannot say what this is doing, but if you consider this piece of code:

def func(lst: List[Account]):
    for thing in lst:
        validate(thing)

You can somewhat tell what this is doing (even if this example is stupid and meaningless).

Another reason why you should have type annotations is to find bugs in your programs before you even run it, for example.

holiday_bonus = "50.0"

def add_bonus(cur_bonus: float):
    return cur_bonus + 50

def should_get_bonus(emp):
    if emp in good_employees:
        emp.bonus = add_bonus(emp.bonus)

As you can clearly see where is the bug, when having type annotations mypy will complain even before running the program, and having to find the specific line that's causing the problem when dealing with larger code base, is just way more difficult rather than running mypy

  • Having type annotations also help when refactoring the code.

Basically, I can't think of a a reason why not too annotate the code, especially when speaking of a code base that's relatively small.

P.S

If you're still not convinced, please watch this, at least the "motivation" and the live demo parts

@joerick
Copy link
Contributor

joerick commented Feb 3, 2020

I've been hanging back from this one, too, in the hope that I might learn something before saying anything that might be interpreted as authoritative! But just thinking out loud...

My past experience with types hasn't been... great... but I probably need more experience. If I was confident this transition would be as simple as scattering some type annotations around, I'd be all-in. But my experience with things like typing.NamedTuple and needing to rearrange to define things before using them has been a little more cumbersome.

cibuildwheel is also pretty light on types - I think right now the project has only 2 classes that are actually passed around. The rest is primitives: strings, dicts, arrays, and None. We don't seem to have a lot of issues resulting from misunderstandings around these.

On the other hand, I'm very familiar with the code, and I know what the variables mean. I think the strongest argument on the other side is how it will help newcomers - whose help is essential to a project like this.

So, all that said - I'd love to be surprised with this! If a type aficionado wants to drop a PR adding the cleanest, neatest types implementation (after the Python 3-only change) I'd love to look at it. I can't say it'll definitely be merged, but if it clearly improves intelligibility, it's got a good chance!

@joerick
Copy link
Contributor

joerick commented Feb 3, 2020

But I was also kind of avoiding answering here, and just hoping @joerick would say "yes"/"no" :D

Sorry - about as non-committal as it gets! haha

@YannickJadoul
Copy link
Member

@YannickJadoul I mean, why not?

Mostly, increased maintenance and tooling cost, I'd say, and because I still can't really see the advantages in this specific case:

  • cibuildwheel offers no public API that would be nicely documented with types
  • We have no weird custom types or deep inheritance hierarchies where e.g. the expected base class becomes important
  • There's a reasonably extended test suite that has so far caught all my stupid mistakes.

So it's not that I can't see how it could be useful, but I find it hard to imagine the benefits for this specific project (vs. the general/abstract examples you give). By the way, in your example

def func(lst):
    for thing in lst:
        validate(thing)

you could also just rewrite as

def validate_all(accounts):
    for account in accounts:
        validate(account)

and basically get the same (but less verbose) information? It feels a bit like a straw man argument, as any code using generic variable names as lst and thing can most likely be improved...

But I think I agree with @joerick's stance. I basically don't have a lot of experience with typing in Python and I probably know most corners of cibuildwheel, so maybe I'm not the right person to judge. If you can point to a location where the names and their usage don't really clarify the expected type, and where explicit types could help, I'd be genuinely interested to see it! Or maybe make a PR and convince us how elegant everything still looks afterwards? :-)

(I've put the video in may list of things to watch, btw; curious to see what I can learn, so thanks for that!)

@Czaki
Copy link
Contributor

Czaki commented Feb 3, 2020

From my perspective (as quite fresh cibuildwheel contributor) I see big profit from typing build function signatures (in macos.py, linux.py, windows.py) This will reduces returning to the __main__.py file with checking what contains this argument. also typing files in util.py could be helpful. Eg. PyCharm show function signature and documenation when clicking Ctrl+Q.

Other option than using Type annotation in signature is to add type annotation in documentation.

@YannickJadoul
Copy link
Member

From my perspective (as quite fresh cibuildwheel contributor) I see big profit from typing build function signatures (in macos.py, linux.py, windows.py)

That's indeed the only shared interface. But I've been thinking for a longer time we need to refactor this. 10+ arguments is just too much, with or without typing.

@Czaki
Copy link
Contributor

Czaki commented Feb 3, 2020

That's indeed the only shared interface. But I've been thinking for a longer time we need to refactor this. 10+ arguments is just too much, with or without typing.

I also thin about this And I think that using kwargs is bad idea. I would rather group it with semantic meaning and use typed NamedTuple for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants