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

Issue #5 - Suppport GPX 1.0 #6

Closed
wants to merge 3 commits into from
Closed

Conversation

hfiguiere
Copy link
Contributor

GPX is still generated by gpsbabel when loading tracks from various devices.

The PR add support for the GPX 1.0 format.

@frewsxcv
Copy link
Member

@brendanashworth any interest in reviewing this? otherwise i can try to find some time this next week

@brendanashworth
Copy link
Member

@hfiguiere thanks for putting this together, apologies for letting it slide through the cracks. I'll be reviewing it over the next few days.

Copy link
Member

@brendanashworth brendanashworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altogether I really like this, just a few questions about approach.

match version_str {
"1.0" => GpxVersion::Gpx10,
"1.1" => GpxVersion::Gpx11,
_ => GpxVersion::Unknown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a requirement to have GpxVersion::Unknown part of the enum? Are we able to always error out if the version 1) isn't provided or 2) is malformed?

I see that the GPX version is required ("You must include the version number in your GPX document."), but perhaps I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an error here is probably better. As for the enum, there is a purpose.

use super::consume;

#[test]
fn consume_gpx() {
let gpx = consume!("<gpx></gpx>");
let gpx = consume!("<gpx></gpx>", GpxVersion::Gpx11);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, this should technically error as it lacks a version=....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is will be handled as an error. My updated PR has a bit more test case for this.

/// consume consumes a single string as tag content.
pub fn consume<R: Read>(reader: &mut Peekable<Events<R>>) -> Result<String> {
pub fn consume<R: Read>(reader: &mut Peekable<Events<R>>, _version: GpxVersion) -> Result<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel as if this version passing becomes an antipattern, this file being a good example. It comes with lots of boiler plate code that, while consistent, isn't clean. What do you think of some sort of larger struct with version data as an alternative?

pub fn consume<R: Read>(context: &mut GpxContext<Peekable<Events<R>>>) -> Result<String> {
  for event in context.reader() {
    // ...
      if (context.version() == GpxVersion::11) { /* ... */ }
    // ...
  }
}

While the signature is ugly (GpxContext<Peekable<Events<Read>>>), I'd genuinely like to hear your opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about context: &mut Context<R: Read> ? I have a patch doing that.

"speed" => {
if version == GpxVersion::Gpx10 {
// Speed is from GPX 1.0
// Ignore.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might this be something we should add to parse-able data, noting it's only for GPX 1.0?

Copy link
Contributor Author

@hfiguiere hfiguiere Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in Waypoint now

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum GpxVersion {
Unknown,
Gpx10,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on alternative naming, like GpxVersion::OneZero, GpxVersion:Gpx1_0? Also, from above, the Unknown value is questionable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Unknown value is needed for consume when we don't know the version yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GpxVersion::Gpx1_0 trigger a warning from the compiler. I don't find GpxVersion::OneZero intuitive.

use GpxVersion;

/// consume consumes an element as a nothing.
pub fn consume<R: Read>(reader: &mut Peekable<Events<R>>, _version: GpxVersion) -> Result<Bbox<f64>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you should run rust-fmt, Travis is complaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This is will be in the updated PR.

@hfiguiere
Copy link
Contributor Author

Updated the PR with most feedback (see inline comments).

Improved some of the syntax and ran rust fmt.

brendanashworth pushed a commit to brendanashworth/rust-gpx that referenced this pull request Mar 10, 2018
brendanashworth pushed a commit to brendanashworth/rust-gpx that referenced this pull request Mar 10, 2018
brendanashworth pushed a commit to brendanashworth/rust-gpx that referenced this pull request Mar 10, 2018
This commit enables support for GPX 1.0 format. It introduces a
"Context" wrapper for version-specific parsing of data.

Fixes: georust#5
PR: georust#6
@brendanashworth
Copy link
Member

@hfiguiere thank you for your contributions! I've merged these in as 6e07049, 9680234, and 385ca1c, and will be sending out a new release over the weekend.

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