georust / gpx

Rust read/write support for GPS Exchange Format (GPX)
https://crates.io/crates/gpx
MIT License
102 stars 46 forks source link

Switch from RFC3339 to ISO8601 for timestamp encoding #78

Closed 0b11001111 closed 2 years ago

0b11001111 commented 2 years ago

Implement changes proposed in https://github.com/georust/gpx/issues/77

I don't expect this to land directly as it adds a breaking change

lnicola commented 2 years ago

r? @urschrei

0b11001111 commented 2 years ago

I added a few more changes to make my linter happy and thereby found a bug in the parsing logic for tracks. On my machine, all unit and integrations tests pass :)

0b11001111 commented 2 years ago

I will later add another test case using the file exported from outdooractive.com that originally caused the RFC 3339 parsing error

lnicola commented 2 years ago

Sorry for taking so long to look at this again, but can you replace the new test file with a smaller version (1-2 points or whatever)? I know that some of the other fixtures are already quite large, but it seems somewhat excessive and maybe we can not add another one?

0b11001111 commented 2 years ago

You're right, I filtered most waypoints out in https://github.com/georust/gpx/pull/78/commits/d3375a47a2eed0e582d52b152b8408a4366066cd

lnicola commented 2 years ago

Thanks. Can you also squash, since there's not much benefit in trimming down the file otherwise? :smile: (I can do it if you don't know how to)

0b11001111 commented 2 years ago

I was today years old when I learned...

Hope I did the right thing :D

lnicola commented 2 years ago

@bors r+

lnicola commented 2 years ago

Huh

bors ping

bors[bot] commented 2 years ago

pong

lnicola commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build failed:

lnicola commented 2 years ago

Ah, this isn't your fault, we need to test with newer compiler versions.

michaelkirk commented 2 years ago

Ah, this isn't your fault, we need to test with newer compiler versions.

I just now opened https://github.com/georust/gpx/pull/80 to address this.

lnicola commented 2 years ago

bors retry

bors[bot] commented 2 years ago

Merge conflict.

michaelkirk commented 2 years ago

I took the liberty of fixing the conflicts in the change log.

bors retry

bors[bot] commented 2 years ago

Build succeeded: