Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request stems from an issue I had as a user of the library. When I added a field of type
GeoPoint2d
to my struct, I discovered the struct could no longer by serialized or deserialized. However, I can add aPoint2d
field to the struct without issue. Why canPoint2d
be serialized and notGeoPoint2d
, especially knowing both contain fields of f64? My preference would be for both to be serializable.The set of common traits that I like to see implemented on public structs in a library, when appropriate for the data type, are Debug, Default, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize and Deserialize. Since you already have
serde
as a dependency, it makes sense to me to derive Serialize and Deserialize where possible, though I also see lots of libraries putting these derives behind a feature gate, and that approach has some appeal to me. Another good candidate for a feature gate are thestrum
and andstrum_macros
libraries which allow you to derive EnumIter on your enums, which can come in handy.Where the data structure does not have a natural Default or cannot be Copy, I did not try to get fancy, I just stripped it from the derive, so the only additions are derives that do not make the compiler complain. The test suite still passes and clippy has no comment the changes. Once I got started on
GeoPoint2d
, I kept going and touched up the structs where I could, including a couple structs that look like internal library housekeeping, where I can't imagine you actually need Hash, likeContourPointsIterator
... maybe that is going too far.There are two somewhat ancient threads I referred to for recommendations on what traits to derive for libraries. User vadixidav makes the pro case on reddit:
User caleblbaker adds the following observation that i find helpful:
On the conservative, or against side, I can quote the venerable Burnt Sushi from the rust users forum:
Generally I would categorize ignoring Burnt Sushi's advice to be foolish, or out on a limb. Note that his primary concern is creating breaking changes for downstream users in the future if these derives need to be removed to accommodate additional constraints introduced by, for example, adding a new field with a more constrictive type. As one of the users described who is coming along saying "Hey you should derive this trait", I kind of see it the other way around. The fact that removing these derives creates fear of breaking changes implies that users are not only taking advantage of these traits, but in fact may rely on them to the extent that removing them could break their code. This is a good thing because it enables us to use to the code in the first place to accomplish the goals of our project.