Allow empty elevation tags #72
Conversation
|
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. |
tests/gpx_read.rs
Outdated
| for segment in &track.segments { | ||
| for point in &segment.points { | ||
| // Elevation should default to 0.00 | ||
| let elevation = point.elevation.unwrap(); |
There was a problem hiding this comment.
I think I'd expect point.elevation.is_none() 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. |
|
If you haven't seen it already - I think you could match on the |
|
Also, thanks for having tests - it makes the PR easier to review when we can see the new behavior in action. 👍 |
|
@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 |
|
I'm happy for them to stay in. If you could update the changelog (you'll need to pull |
|
bors r+ |
72: Allow empty elevation tags r=urschrei a=ebcrowder Similar to #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](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGELOG.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Eric Crowder <eric@ebcrowder.dev>
src/parser/waypoint.rs
Outdated
| waypoint.elevation = Some(string::consume(context, "ele", false)?.parse()?) | ||
| waypoint.elevation = match string::consume(context, "ele", false) { | ||
| Ok(v) => Some(v.parse()?), | ||
| Err(_) => None, |
There was a problem hiding this comment.
Aren't there other errors that we'd want to surface here? I think the only error we'd want to map to None is GpxError::NoStringContent
Err(GpxError::NoStringContent) => None,
Err(other_err) => return Err(other_err)
For example, I think we'd want an element like this to continue to error, not return None:
<ele>1.asdf</ele>
|
bors r- |
|
bors r- (sorry!) |
|
Canceled. |
|
bors r+ |
|
Build succeeded: |
Similar to #70, I have a GPX file that has self-closing
elevationtags, 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
f64value of0.00for 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.CHANGELOG.mdif knowledge of this change could be valuable to users.