joey9801 / igc-rs Goto Github PK

Looking at https://github.com/Turbo87/rust-igc, it seems like this may be a fork of https://github.com/Turbo87/rust-igc, is that true?

The IGC file spec specifies that altitudes can be negative (necessary e.g. in the Netherlands). That means u16 would have to change to i16. That however would shrink the range of usable positive values in an unreasonable amount, so I would suggest to bite the bullet and use i32 instead.

It seems that this crate implements an older variant of the A record format. The AFAIK most recent variant is specified on page 22 of https://github.com/Turbo87/rust-igc/blob/master/docs/IGC_FR_Spec_with_AL4a_2016-4-10.pdf as A MMM NNN (instead of A M NNNNN). There are also a few other variants commonly in use that encode other data in there. We might have to read the first M byte, and figure out from that what variant to use.

The crate claims that it is low-level, but it seems that the Date parser makes an assumption which goes against that claim. I would suggest that the low-level parser should only parse the year as a two-digit number and that a higher-level parser could then figure out the correct date on top of that.

If we keep the code like this I would suggest to at least use e.g. 1980 as the baseline, as there are plenty of IGC files from before 2000 around.

The latest version on crates.io is not up to date and is therefore missing essential things like Clone and Serde features.
Latest version on crates.io is from september 2019.

The docs state:

This crate provides a minimal, fast parser for IGC files.

But from the docs, it also seems that it only provides a line/record parser. Or did I overlook something?

I'm aware that I can parse a file by mapping the parsing function on a line iterator, but it would be cool if there were a function to do that in this library (e.g. a parser that can process any type that implements std::io::Read). What do you think?

While the README claims that this is only a parser, the low-level nature of the structs seems to make it relatively straight-forward to implement writers for them too. Have you thought about that yet, or is that out-of-scope for this crate?

I'm asking because of the discussion in Turbo87/ogn-web-gateway#11

Over at https://github.com/Turbo87/rust-igc I had started to use https://github.com/altsysrq/proptest for some basic input testing. It might be useful to implement something similar here.

It would be nice if igc::util::ParseError would implement std::error::Error.

Is there any specific reason why you didn't derive Copy + Clone? Copy is kind of controversial in the way that removing it is a breaking change and if you forsee that the library might not be able to provide Copy implementations in the future I wouldn't suggest deriving it.

But I do not see any reason to not implement Clone for example. serde-related derives could also be added behind a serde feature flag (which is actually automatically added by cargo if you happen to declare your serde dependency as optional).

Would you be open to accept such a PR?