#175 Restore timezone aware datetime objects when time specified with Z (zulu) #186
Conversation
|
I also had to modify the CI setup as newer setuptools versions conflict with something in the setup.py or related code. I wasn't quite able to rootcause it. I opted to just fix an older setuptool versions, as the least intrusive change. A bigger and more longerm solution is probably to move away from setuptools and use a pyproject.toml file instead. That's outside the scope of this PR. |
| if utcdelta: | ||
| self._utcdelta = utcdelta | ||
| else: | ||
| self._utcdelta = datetime.datetime.now() - self._now |
There was a problem hiding this comment.
I don't know what this self._utcdelta piece of code did. It does not seem to be referenced anywhere else in the codebase. I kept the argument to not break backwards compatibility, but removed the dead code.
metar/__init__.py
Outdated
| __email__ = "pollard@alum.mit.edu" | ||
|
|
||
| __version__ = "1.11.0" | ||
| __version__ = "1.12.0" |
There was a problem hiding this comment.
Why is the version being bumped?
There was a problem hiding this comment.
Sorry, I assumed that had to be done do make a new PyPI release. I'm unfamiliar with whether that's commonly done separately. Now removed from this PR.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 88.62% 88.94% +0.31%
==========================================
Files 4 4
Lines 1046 1040 -6
==========================================
- Hits 927 925 -2
+ Misses 119 115 -4 ☔ View full report in Codecov by Sentry. |
|
I am on the fence on this. Presently, we have an unambiguous situation of always having a naive datetime returned. This can be important for downstream consuming apps that can't handle a mix of naive and not naive. I'd rather leave the datetime as naive, but keep your _zulu bool, for those to use downstream. Others have thoughts? |
Indeed, as was found with #184, a PR that does this would be very appreciated! |
|
Has anyone seen a METAR report with a timestamp that isn't suffixed with Z? I've never encountered one in my flying career, I've only seen ones with Z suffix. I am unable to follow the README links to any of the METAR specs, so the best reference I've found is this random PDF https://www.weather.gov/media/wrh/mesowest/metar_decode_key.pdf , which on page 5 says "DATE/TIME All dates and times in UTC using a 24-hour clock; two-digit date and four-digit time; always appended with Z to indicate UTC.". It seems unlikely to be allowed, but I opted to keep that support in this PR since the original regexp had the Z as optional. I agree that its a bit messy that the library now can return both. It's important to note that v1.10.0 of this library always returned UTC tzinfo datetime. Only in v1.11.0 was this (accidentally?) changed as part of removing the deprecated utcnow() function, see 7dfbe07 . |
This relates to the fix for issue python-metar#175, commit 7dfbe07, which accidentally changes the datetime objects from python-metar from timezone aware to non-timezone-aware. A new field ._zulu is added to distinguish whether the Z char is used as suffix of the timestamp in the METAR, but the timestamp is always assumed to be in UTC.
|
I've updated the pull request to now always return UTC timezone datetime object, returning the behavior to the v1.10.0 revision. Thoughts? The patch is also rebased on top of the pyproject.toml change in #187. |
|
I don't think your assessment of 7dfbe07 is right >>> import datetime
>>> datetime.datetime.utcnow().tzinfo is None
True |
|
For the record, I am sort of in favor of this change, but I suspect it will cause downstream folks grief. |
The fix for issue #175 , commit 7dfbe07, changes the returned datetime objects to no longer have timezone information. This makes it harder to use this library as it becomes arguably harder to compare times against other datetime objects that do have timezone information. The timezone of a METAR is well defined (at least if the Z char is present), so it makes sense in my eyes that the returned object should be timezone aware.