consolidate WeeklySchedule (closes #19) #27
Conversation
basil96
changed the title
|
GitHub complains about conflicts in this one now. I don't understand why. When I pull this to a separate clone and checkout this branch, I see no issues. I'd like to perform this merge as an interactive rebase from client side this time instead of using GH. |
|
Ahh, nvm, I have to merge master into this branch first because master had a commit while this was in progress. Why isn't this workflow easier to grasp.. |
|
Having no fun at all with resolving this merge conflict in GitKraken... this PR is on hold until I figure that out and merge master into this branch. |
|
It's ready for review now. |
| tzf = TimezoneFinder() | ||
| self.tzinfo = pytz.timezone(tzf.timezone_at( | ||
| lat=observer.latitude, lng=observer.longitude)) | ||
| self.tzinfo = tzinfo |
There was a problem hiding this comment.
should this be in an else? seems like if tzinfo is one, we set self.tzinfo, then the next line resets it to None.
Is there a test for this? I'll look maybe I can write one
There was a problem hiding this comment.
Ha! You're right, I made a bug. But after some investigation, I found something interesting..
In this context, calls like astral.sun.twilight() result in astral calling .astimezone(tzinfo) on the given datetime object. When tzinfo is None, the call effectively becomes something like:
>>> pytz.utc.localize(datetime.datetime.utcnow()).astimezone(None)
which returns:
datetime.datetime(2020, 5, 9, 1, 25, 52, 659647, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000), 'Eastern Daylight Time'))
According to Python docs on datetime.astimezone(), it picks up the timezone from the OS in that case:
"If called without arguments (or with tz=None) the system local timezone is assumed for the target timezone. The .tzinfo attribute of the converted datetime instance will be set to an instance of timezone with the zone name and offset obtained from the OS."
That's why it still produces the correct sunsets.csv.
Given this development, we don't need timezonefinder or its friend numpy. I think relying on the correct timezone/locale setup in a client's OS is faster and more reliable.
This means I can remove the entire if block and remove timezonefinder from the project.
There's no unit test for this, you should write one. My only quick test during development was to change lat/lng values significantly and then wondering why sunsets.csv was still the same.
There was a problem hiding this comment.
Another thought: we could follow astral's convention and change the default tzinfo to pytz.utc. Then a caller would have to explicitly pass None if the local system timezone is desired.
* If timezone is not given, do not calculate it from location. Default to UTC. If `None` is given, Python will use the calling system's local timezone. * Remove timezonefinder from project.
|
Done, tzf is gone and #30 can be started. |
See changeset. Tested fully on Linux.