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.

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

Already on GitHub? Sign in to your account

Add subsecond and timezone support #160

Closed
wants to merge 7 commits into from
Closed

Conversation

fgouget
Copy link
Contributor

@fgouget fgouget commented Dec 30, 2018

This branch includes my previous subsecond support and adds support for returning the TimeZone information in the DateTime objects.
As noted in the commit this means gpxpy may now return timezone aware DateTime objects which means applications that try to mix them with naive DateTime objects will need to be modified.

Another approach with less potential for breaking applications would be to systematically do the conversion to UTC and keep returning naive DateTime objects. But this means the application would still not know what timezone was initially used.

Or one could make this configurable so only timezone aware applications get timezone aware DateTime objects. But this would require more drastic changes to the architecture so TimeConvert can know what to do.

Combining the Z suffix (for UTC) with a timezone offset makes no sense.
Note that this did not cause the tests to fail because the time parsing
code strips both anyway but the tests should use a valid syntax.

Signed-off-by: Francois Gouget <fgouget@free.fr>
Signed-off-by: Francois Gouget <fgouget@free.fr>
Signed-off-by: Francois Gouget <fgouget@free.fr>
Test the timestamp parsing function through TimeConverter class which is
the actual interface used by gpxpy. This allows testing the conversion
of DateTime objects back to strings.
Remove the code that's stripping the trailing 'Z' from the test
strings: it's shorter and clearer to just add the relevant tests.
For each timestamp store the actual DateTime components we're expecting
so we can more precisely check the result of the conversion.
Add more variants of the timestamp formats we want to support.

Signed-off-by: Francois Gouget <fgouget@free.fr>
The time parsing code was losing the second decimals. When the GPX file
is used to interpolate the position of photos based on their timestamp
this can lead to a loss of precision proportional to the speed at which
the camera is moving, that is as much as 30 meters at highway speeds.
Only drop the '+HH:MM' and 'Z' timezone specifications when they are at
the end of the string and truncate the second decimals to microsecond
precision to accomodate datetime's strptime() function.
Modify test_parse_time() so it verifies the returned datetime object,
including the second decimals unlike test_long_timestamps()!
Modify TimeConverter.to_string() so it preserves the sub-second data but
preserves the old format if none is specified.

Signed-off-by: Francois Gouget <fgouget@free.fr>
It has simply been renamed to assertEqual().

Signed-off-by: Francois Gouget <fgouget@free.fr>
Applications that use GPX files to geolocalize other data (e.g. photos)
based on the timestamps must know which timezone each of them is basedi
on. So return the timezone information if specified in the timestamp.

Note that this changes the gpxpy API in that timezone aware DateTime
objects cannot be directly compared with naive DateTime objects. This
means applications that only dealt with naive DateTime objects will now
need to at least be ready to strip the tzinfo field of DateTime objects
returned by gpxpy.

Signed-off-by: Francois Gouget <fgouget@free.fr>
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.707% when pulling 7458ba9 on fgouget:master into 5ae0fee on tkrajina:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.707% when pulling 7458ba9 on fgouget:master into 5ae0fee on tkrajina:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.707% when pulling 7458ba9 on fgouget:master into 5ae0fee on tkrajina:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.707% when pulling 7458ba9 on fgouget:master into 5ae0fee on tkrajina:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.707% when pulling 7458ba9 on fgouget:master into 5ae0fee on tkrajina:master.

@tkrajina
Copy link
Owner

Hm, from a quick glance at this PR, looks like #152 but without timezones support. Can you check #152 and see if that fixes your use case?

@tkrajina
Copy link
Owner

tkrajina commented Mar 2, 2019

Implemented in #152

@tkrajina tkrajina closed this Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants