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

Switch from TryFrom to FromStr for Manifest #107

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Switch from TryFrom to FromStr for Manifest #107

merged 2 commits into from
Oct 10, 2023

Conversation

xfbs
Copy link
Contributor

@xfbs xfbs commented Oct 10, 2023

This PR changes Manifest and RawManifest to implement the FromStr trait rather than the TryFrom<String> trait.

While both traits can be used and do achieve a similar purpose, there is one important difference in their semantics related to what they do with the string they are taking:

  • The TryFrom<String> attempts to consume a String and convert it into the target type (Manifest or RawManifest). The ownership of the String is thereby moved during the conversion. This is therefore useful if you do need ownership of the input type.
  • The FromStr trait attempts to parse a &str (reference to a string) into the target type (Manifest or RawManifest). It does not take ownership of the string, only a read-only reference to it. This is therefore useful if you only need to read the input string.

Since fundamentally, are are just parsing a string and we do not need it anymore after the parsing operation, the FromStr trait is more useful here because it does not require us to consume to the string. The TryFrom<String> trait would be more appropriate if we wanted to hold on to the original string after the parsing operation, and perhaps store that inside the Manifest type.

But since we simply discard the string, there is no good reason to use TryFrom. For this reason, this PR changes the code base to use FromStr instead.

@xfbs xfbs requested a review from asmello October 10, 2023 09:12
@xfbs xfbs self-assigned this Oct 10, 2023
src/manifest.rs Outdated Show resolved Hide resolved
src/package.rs Outdated Show resolved Hide resolved
@mara-schulke
Copy link
Contributor

Thank you 😊 good catch!

parse allows call chains, whereas from_str requires nesting
@xfbs
Copy link
Contributor Author

xfbs commented Oct 10, 2023

Good point! I do prefer parse() as well. I changed all invocations of from_str() to parse().

@mara-schulke mara-schulke added the type::style Related to personal or community opinions label Oct 10, 2023
@mara-schulke mara-schulke added the complexity::low Issues or ideas with a low implementation cost label Oct 10, 2023
@mara-schulke mara-schulke merged commit acd5f27 into main Oct 10, 2023
@mara-schulke mara-schulke deleted the pe/manifest branch October 10, 2023 10:49
@xfbs xfbs restored the pe/manifest branch October 12, 2023 09:08
@mara-schulke mara-schulke deleted the pe/manifest branch November 3, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::low Issues or ideas with a low implementation cost type::style Related to personal or community opinions
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants