Skip to content

Make zoneinfo tzfile superclass less confusing#485

Merged
pganssle merged 1 commit into
dateutil:masterfrom
quodlibetor:tzfile-superclass-clarity
Oct 27, 2017
Merged

Make zoneinfo tzfile superclass less confusing#485
pganssle merged 1 commit into
dateutil:masterfrom
quodlibetor:tzfile-superclass-clarity

Conversation

@quodlibetor

Copy link
Copy Markdown
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
Copy Markdown
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