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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Update stubs for pytz #3393

Merged
merged 3 commits into from
Oct 30, 2019
Merged

Update stubs for pytz #3393

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86e4a12
pytz, FixedOffset
angelaambroz Oct 20, 2019
4951896
Removing private kwarg
angelaambroz Oct 22, 2019
81cef77
MR comments
angelaambroz Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion third_party/2and3/pytz/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional, List, Set, Mapping, Union
from typing import Optional, List, Set, Mapping, Union, Dict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to remove the Dict.

import datetime

class BaseTzInfo(datetime.tzinfo):
Expand Down Expand Up @@ -29,6 +29,7 @@ class NonExistentTimeError(InvalidTimeError): ...
utc: _UTCclass
UTC: _UTCclass
def timezone(zone: str) -> Union[_UTCclass, _StaticTzInfo, _DstTzInfo]: ...
def FixedOffset(offset: int) -> Union[_UTCclass, _StaticTzInfo, _DstTzInfo]: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like FixedOffset is an ad-hoc implementation.

If offset is 0, it returns a _UTCclass.

If offset is not 0, it returns a class _FixedOffset. This class is not a subclass of BaseTzInfo, though it is compatible with its interface, except for zone being None. It also implements utcoffset (without a is_dst argument), dst and tzname (returning None). So it doesn't fit with any of the existing classes.

A bit of a mess! I suppose you can add a _FixedTzInfo class which encodes all of that, not sure if it's worth it. Otherwise, you should change the type to Union[_UTCclass, tzinfo].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with bluetech here. The current return type seems suboptimal, because we generally discourage Union return types, and in fact it doesn't ever return a _StaticTzInfo or a _DstTzInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 Thanks, both. And apologies for the silence - got sucked into other work. I can change the return type to Union[_UTCclass, tzinfo], per bluetech's comment. Will make the change, run the tests locally, and push it up now.


all_timezones: List[str]
all_timezones_set: Set[str]
Expand Down