search

Joey9801 / igc-rs

MIT License
6 stars 4 forks source link

Reason for not implementing common Rust traits #37

Closed mainrs closed 3 years ago

mainrs commented 4 years ago

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?

mainrs commented 4 years ago

On that note: https://rust-lang.github.io/api-guidelines/interoperability.html

Joey9801 commented 4 years ago

No strong reason for the lack of Clone/Copy impls - just that this was the first Rust library I'd released and didn't think to do it. I'm happy to add Clone impls for all the record+util types that don't have it already and release a patch version.

Regarding Copy - Most of the utility types already do implement Copy. I and K records can include heap-allocated data in their Vec of extension definitions, so they shouldn't implement Copy. This means that the general igc_rs::records::Record<'a> enum shouldn't implement Copy either. Given that, I'm not sure it's worth making the other record types Copy.

I'm not so sure Serde makes that much sense for this library though, the IGC format doesn't really map to the Serde data model, so one could never reasonably use Serde to parse/reproduce IGC files. If you were after being able to serialize the parsed records into other Serde compatible formats (eg json), I guess you could do that, but I don't really see the use case. If you're looking to use this parser in other non-rust languages, I'd wager that it would be better to just use this library directly with some FFI, rather than use it to preprocess the IGC file into another format such like JSON. To this end I'd happily accept the addition of a C API to this project / bindings to other languages.

mainrs commented 4 years ago

Given that, I'm not sure it's worth making the other record types Copy.

I didn't take a close look at the types but this could be something that can be implemented if more people are interested in it.

If you were after being able to serialize the parsed records into other Serde compatible formats (eg json), I guess you could do that, but I don't really see the use case.

Yep, this was the idea. I don't have a use-case, but maybe others do have. Adding them behind a feature doesn't add any costs to compilation for all existing users and can be useful for others, so I'd personally go for it. If you want/are ok with it I can open a PR later on today for this.

Joey9801 commented 4 years ago

I guess there's no harm in sprinkling a few #[cfg_attr(feature="serde", derive(Serialize, Deserialize))] lines around. Though if you start needing to replace &'a str's with Cow<'a, str>'s to get lifetimes to work I would say it's not worth the breaking API change.

mainrs commented 4 years ago

Can you assign this issue to me please? I will get to it next week :)