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

feat: into_v1 #121

Merged
merged 2 commits into from
Aug 8, 2022
Merged

feat: into_v1 #121

merged 2 commits into from
Aug 8, 2022

Conversation

Arqu
Copy link
Contributor

@Arqu Arqu commented Aug 3, 2022

Simple conversion based on what other implementations do.
Should also help move #59 along.

@dignifiedquire dignifiedquire requested a review from vmx August 3, 2022 21:09
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Could you please add tests for into_v1?

I also thought I leave a nitpick as when you're adding tests you need to touch the code anyways :)

src/cid.rs Outdated
@@ -108,6 +108,19 @@ impl<const S: usize> Cid<S> {
}
}

/// Convert a CIDv0 to a CIDv1
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick (change will be approved and merge even without changing it): Perhaps adding information that if it is already a CIDv1, that it will be unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I literally wrote it out, thought it was a bit verbose compared to the others and dropped it. Also test incoming :)

@Stebalien Stebalien merged commit 17e37cb into multiformats:master Aug 8, 2022
@Arqu Arqu deleted the arqu/into_v1 branch August 20, 2022 09:16
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