Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

derive Hash for most pub types that also derive PartialEq #852

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

bruceg
Copy link
Contributor

@bruceg bruceg commented Oct 20, 2022

We needed to store Item alongside other data in a hash set, which requires all of the contained data be hashable. While I was adding the derive for that type, I looked for other pub types that could use it.

@djc djc requested a review from esheppa October 21, 2022 11:23
Copy link
Collaborator

@esheppa esheppa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Nice to see the format::Item API being used.

@bruceg, please let us know if there are any documentation improvements needed in the format::Item and format::StrftimeItems API

@djc djc merged commit d147e9a into chronotope:main Oct 22, 2022
@djc
Copy link
Member

djc commented Oct 22, 2022

Probably want to backport this to 0.4.x, @bruceg do you want to submit that?

@bruceg
Copy link
Contributor Author

bruceg commented Oct 23, 2022

A good example of how it can be used would be useful for documentation, ie feeding it in to format_with_items or what have you.

A bigger issue we had with this type was ownership. When parsing a string, we get items that reference the same lifetime as the string. When we later want to collect those items into a vec and store it for repeated use, we had to convert those into Item<'static> by boxing all the Literals and Spaces into OwnedLiteral and OwnedSpace. A better interface, that mimics other Rust types, might be to drop the owned variants from Item, but have a fn to_owned(&self) -> OwnedItem which has all boxed variants. I haven't looked at the underlying code to see if this makes sense for all your uses, though.

Funny you should mention 0.4.x. We use that for our releases, so we already did backport the change there. I will submit that after the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants