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

Implement previous iterator for subpaths #70

Merged
merged 36 commits into from
Oct 24, 2022
Merged

Conversation

matheus23
Copy link
Member

@matheus23 matheus23 commented Oct 14, 2022

WIP because:

  • Documentation
  • Clearer code. (Maybe some ideas for cleaning up? Perhaps let-else statements?)
  • Add reference to PrivateForest in PrivateNodePathHistory?
  • WASM bindings?
  • git conflicts

@matheus23 matheus23 self-assigned this Oct 14, 2022
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #70 (36016b0) into main (c88a0f8) will increase coverage by 0.33%.
The diff coverage is 66.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
+ Coverage   64.29%   64.62%   +0.33%     
==========================================
  Files          24       25       +1     
  Lines        1613     1795     +182     
  Branches      416      458      +42     
==========================================
+ Hits         1037     1160     +123     
- Misses        213      233      +20     
- Partials      363      402      +39     
Impacted Files Coverage Δ
crates/fs/common/error.rs 0.00% <ø> (ø)
crates/fs/private/node.rs 67.25% <0.00%> (-3.31%) ⬇️
crates/fs/private/key.rs 58.33% <50.00%> (ø)
crates/fs/private/previous.rs 70.11% <70.11%> (ø)
crates/fs/private/hamt/node.rs 64.41% <0.00%> (+0.61%) ⬆️

@matheus23 matheus23 marked this pull request as ready for review October 17, 2022 18:32
@matheus23
Copy link
Member Author

Flipping this to ready-for-review @appcypher, because I'm thinking this is already really useful & the wasm piece is probably another PR in and of itself, and I should probably have webnative & rs-wnfs/wasm side-by-side while working on that (i.e. it'll take a little more time).

Do you think that makes sense?

@appcypher
Copy link
Member

Yep. Makes sense to me.

Copy link
Member

@appcypher appcypher left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 🎉. We are step closer to feature parity.
I have a few refactor comments.

Copy link
Contributor

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

Starter set of comments.

crates/fs/private/previous.rs Outdated Show resolved Hide resolved
crates/fs/private/previous.rs Show resolved Hide resolved
crates/fs/private/previous.rs Outdated Show resolved Hide resolved
crates/fs/private/previous.rs Outdated Show resolved Hide resolved
crates/fs/private/previous.rs Show resolved Hide resolved
crates/fs/private/previous.rs Show resolved Hide resolved
crates/fs/private/previous.rs Outdated Show resolved Hide resolved
crates/fs/private/previous.rs Show resolved Hide resolved
crates/fs/private/previous.rs Outdated Show resolved Hide resolved
crates/fs/private/previous.rs Show resolved Hide resolved
Copy link
Contributor

@zeeshanlakhani zeeshanlakhani left a comment

Choose a reason for hiding this comment

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

need a rebase from main, but 👍🏽. I updated an outdated comment, but no biggie there either (sorry for not realizing the node history was already in the of constructor). Thanks for all the updates.

Copy link
Member

@appcypher appcypher left a comment

Choose a reason for hiding this comment

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

Let's go! 🎉

@matheus23 matheus23 merged commit 30b180c into main Oct 24, 2022
@matheus23 matheus23 deleted the matheus23/private-previous branch October 24, 2022 13:08
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