Skip to content

Conversation

@quodlibetor
Copy link
Contributor

Specifically, this was confusing to mypy.

The new code is slightly easier to understand if you don't know what's going
on, IMO, otherwise I wouldn't necessarily change valid upstream code to appease
a typechecker.

Specifically, before this change:

$ mypy dateutil/zoneinfo/__init__.py | grep zoneinfo
dateutil/zoneinfo/__init__.py:18: error: Name 'tzfile' already defined (possibly by an import)
dateutil/zoneinfo/__init__.py:18: error: Cycle in inheritance hierarchy

That Cycle inheritance one is bad enough to cause mypy to give an error even
with --follow-imports=silent, which might be a bug in mypy, but, like I said,
I think the new code is more obvious to humans.

Specifically, this was confusing to mypy.

The new code is slightly easier to understand if you don't know what's going
on, IMO, otherwise I wouldn't necessarily change valid upstream code to appease
a typechecker.

Specifically, before this change:

$ mypy dateutil/zoneinfo/__init__.py | grep zoneinfo
dateutil/zoneinfo/__init__.py:18: error: Name 'tzfile' already defined (possibly by an import)
dateutil/zoneinfo/__init__.py:18: error: Cycle in inheritance hierarchy

That Cycle inheritance one is bad enough to cause mypy to give an error even
with `--follow-imports=silent`, which might be a bug in mypy.
@pganssle
Copy link
Member

This is very reasonable. I will merge when the tests pass.

@pganssle pganssle merged commit a978004 into dateutil:master Oct 27, 2017
@quodlibetor quodlibetor deleted the tzfile-superclass-clarity branch October 27, 2017 19:04
@pganssle pganssle mentioned this pull request Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants