Allow empty elevation tags

[ebcrowder]

Similar to https://github.com/georust/gpx/issues/70, I have a GPX file that has self-closing elevation tags, which of course, contain no value. This appears to be valid per the GPX spec at https://www.topografix.com/GPX/1/1/#type_ptType.

The GPX file was generated by a Wahoo bike computer.

This change edits the parsing logic to use a default f64 value of 0.00 for these cases. I didn't see a precedence for handling similar optional values in this lib, so let me know if I should handle this differently in accordance with the project's goals.

• [x] I agree to follow the project's code of conduct.

• [x] I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

[urschrei]

If I'm understanding this change correctly, wouldn't that result in spurious elevation values of 0.0? Unless I'm misunderstanding the spec (and I may well be), we should parse then discard empty elevation tags – they don't represent any useful information. I should qualify my opinion by noting that I haven't done much work with GPX, so very happy to be corrected here.

[ebcrowder]

If I'm understanding this change correctly, wouldn't that result in spurious elevation values of 0.0? Unless I'm misunderstanding the spec (and I may well be), we should parse then discard empty elevation tags – they don't represent any useful information. I should qualify my opinion by noting that I haven't done much work with GPX, so very happy to be corrected here.

Yes! Good point. 0.00 would be a valid elevation value, so defaulting to that would not be a good approach. I will rework this to parse and discard those tags.

[michaelkirk]

If you haven't seen it already - I think you could match on the Err(GpxError::NoStringContent) case returned from string::consume(context, "ele", false)

[michaelkirk]

Also, thanks for having tests - it makes the PR easier to review when we can see the new behavior in action. 👍

[ebcrowder]

@michaelkirk @urschrei thanks for the feedback. I made those edits.

Even though they don't explicitly cover this change, I did maintain the additional tests at src/parser/string.rs since I think they provide some additional clarity of the string parsing behavior. I'd be happy to remove them if you prefer, though.

[urschrei]

I'm happy for them to stay in. If you could update the changelog (you'll need to pull master first since we've merged some changes) we'll get this merged today and push out a bugfix release, since this is the second spec-related bug that's been fixed.

[urschrei]

bors r+

[urschrei]

bors r-

[michaelkirk]

bors r-

(sorry!)

[bors[bot]]

Canceled.

[michaelkirk]

bors r+

[bors[bot]]

Build succeeded:

• ci result