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

Support Number on Tracks #57

Merged
merged 3 commits into from Aug 8, 2021
Merged

Support Number on Tracks #57

merged 3 commits into from Aug 8, 2021

Conversation

tehsmeely
Copy link
Contributor

@tehsmeely tehsmeely commented Aug 8, 2021

GPX 1.1 Spec includes an optional Number on Track elements, which support is added for here.
Some Gpx files exported from google maps appear to include this so included test + test gpx file from that are used to verify change.

N.b. Number, as a u8, was already present and commented out on the Track struct - in case there was some history there being dragged up (It was already commented out during a refactor 4 years ago and that's as far back as i looked ...)

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

Added massive lands-end-to-johnogroats gpx as test for track numbers.
@tehsmeely tehsmeely changed the title (re)add number to Track struct. Parse number to u32 optionally on tracks Support Number on Tracks Aug 8, 2021
@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

Can you also update the changelog?

bors r+ d+

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

🔒 Permission denied

Existing reviewers: click here to make lnicola a reviewer

@tehsmeely
Copy link
Contributor Author

tehsmeely commented Aug 8, 2021

Changelog and Cargo version updated

@urschrei
Copy link
Member

urschrei commented Aug 8, 2021

@lnicola try again when you're ready.

@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

Hmm, do you think it's worth removing some of the data from that trace? I haven't looked at the file (not at the others), but it might be a good idea to use small test cases.

@tehsmeely
Copy link
Contributor Author

That's a very reasonable ask, I'll generate a smaller gpx sample file for this test and update here

@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

I'm also worried about potential privacy implications, but maybe that's not the case here.

@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

bors d+

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

✌️ tehsmeely can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@tehsmeely
Copy link
Contributor Author

I'm also worried about potential privacy implications, but maybe that's not the case here.

Also a good point, both are generic "random" routes exported from google maps not pointing to any locations that should pose privacy concerns


Updated test files

@tehsmeely
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

Build succeeded:

@bors bors bot merged commit cc0ef84 into georust:master Aug 8, 2021
@tehsmeely
Copy link
Contributor Author

Thanks Folks!

@lnicola
Copy link
Member

lnicola commented Aug 8, 2021

Next time please squash after replacing large files 😬.

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.

None yet

3 participants