Skip to content
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

Type Hint for ipaddress.collapse_addresses inconsistent with other functions in the module causing false positives #2080

Closed
exhuma opened this issue Apr 25, 2018 · 1 comment

Comments

@exhuma
Copy link
Contributor

exhuma commented Apr 25, 2018

The following script will not type-check, but it should:

from ipaddress import ip_network, collapse_addresses


nets = [
    ip_network('192.0.2.0/25'),
    ip_network('192.0.2.128/25'),
]

collapse_addresses(nets)

The reason is that collapse_addresses uses the type variable _N whereas ip_network returns Union[IPv4Network, IPv6network]

The both type hint definitions look reasonable. But unfortunately it will cause error messages when using mypy because _N is not the same as Union[IPv4Network, IPv6Network].

I was briefly running this through my head:

  • Changing collapse_addresses to use "Union" as well could cause false negatives for iterables of mixed types.
  • Changing ip_network to return _N might wreak havoc on existing code-bases
  • Replacing _N with two overloaded type-hints for collapse_addresses might work?

I'm not quite sure how to address this. It might even make sense to not change this at all and force users of the ipaddress to address this on the "client-side"?

I'm willing to open a PR and test it against my existing code-base which uses ipaddress quite heavily and I am in the process of adding type-hints. But I first wanted to get some feedback as to which course of action was appropriate.

@JelleZijlstra
Copy link
Member

I think the best (if unfortunate) solution is to make ip_address return Any. We generally try to avoid Union return types, as mentioned in https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#conventions, because they usually end up forcing awkward code for users.

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

No branches or pull requests

3 participants