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: Remove Share struct, add documentation, add rc constructor variants #343

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

appcypher
Copy link
Member

Summary

This PR adds and updates existing documentation, doc tests and makes other API clean ups.

  • Add more information to docs home page
  • Remove unused types and Share struct
  • Update READMEs
  • Add rc constructor variants

Test plan (required)

  • Testing the code.

    scripts/wnfs.sh test

Closing issues

@appcypher appcypher requested a review from a team as a code owner August 30, 2023 15:31
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #343 (549cec6) into main (2d15fbd) will decrease coverage by 0.25%.
The diff coverage is 36.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
- Coverage   57.08%   56.83%   -0.25%     
==========================================
  Files          45       45              
  Lines        3346     3336      -10     
  Branches      836      833       -3     
==========================================
- Hits         1910     1896      -14     
- Misses        874      883       +9     
+ Partials      562      557       -5     
Files Changed Coverage Δ
wnfs-common/src/metadata.rs 24.52% <ø> (ø)
wnfs/examples/file_variants.rs 0.00% <0.00%> (ø)
wnfs/examples/mnemonic_based.rs 0.00% <0.00%> (ø)
wnfs/examples/private.rs 0.00% <0.00%> (ø)
wnfs/examples/public.rs 0.00% <0.00%> (ø)
wnfs/examples/tiered_blockstores.rs 0.00% <0.00%> (ø)
wnfs/examples/write_proofs.rs 0.00% <0.00%> (ø)
wnfs/src/lib.rs 90.00% <ø> (ø)
wnfs/src/private/file.rs 73.39% <0.00%> (-2.22%) ⬇️
wnfs/src/private/forest/traits.rs 100.00% <ø> (ø)
... and 11 more

... and 1 file with indirect coverage changes

@appcypher appcypher changed the title Cleanup chore: cleanups and doc Aug 30, 2023
Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Big fan of most changes, just not a big fan of saving a bunch of characters with the rc name.
Wdyt about new_rc? Or perhaps an rc function with the signature fn rc(self) -> Rc<Self>? So it's &mut PrivateDirectory::new(...).rc() (perhaps that's too easy to miss in examples though)?

wnfs/src/lib.rs Show resolved Hide resolved
@appcypher
Copy link
Member Author

appcypher commented Aug 31, 2023

Or perhaps an rc function with the signature fn rc(self) -> Rc? So it's &mut PrivateDirectory::new(...).rc() (perhaps that's too easy to miss in examples though)?

Thought about making an rc function; probably should go with that.
I know this change adds little but I think it simplifies the interface makes it less verbose. We almost always wrap these types in Rcs; they have little use outside of that so I think it only makes sense to make it part of the API.
Also, it is in line with functions like Box::pin that wraps a Box in Pin. There are similar functions like this in the ecosystem.

@matheus23
Copy link
Member

Also, it is in line with functions like Box::pin that wraps a Box in Pin. There are similar functions like this in the ecosystem.

hmm I see. I'd argue Box is just inherently a simpler type (& Box::new thus a simpler function), since it's just a wrapper.
The constructor for PrivateDirectory::new takes multiple arguments and is more complex than a wrapper.

What do you think of calling the function new_rc?

Also. Perhaps worth trying this out: How crazy would it be if new simply always returned an Rc? Do we have code in rs-wnfs that would break? Maybe we can base our whole API around Rc<PrivateDirectory> and &mut Rc<PrivateDirectory>...

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

Addresses some of the #315

@matheus23 matheus23 changed the title chore: cleanups and doc feat: Remove Share struct, add documentation, add rc constructor variants Sep 4, 2023
@matheus23 matheus23 merged commit e6cee87 into main Sep 4, 2023
10 checks passed
@matheus23 matheus23 deleted the appcypher/cleanup branch September 4, 2023 12:31
@github-actions github-actions bot mentioned this pull request Sep 4, 2023
@appcypher
Copy link
Member Author

@matheus23 Thanks for fixing 👍. Was going to get to it today.

@github-actions github-actions bot mentioned this pull request Sep 10, 2023
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.

2 participants