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

traverse walk function added with tests #127

Merged
merged 1 commit into from
Nov 29, 2021
Merged

traverse walk function added with tests #127

merged 1 commit into from
Nov 29, 2021

Conversation

patrickwoodhead
Copy link
Collaborator

@patrickwoodhead patrickwoodhead commented Nov 19, 2021

This PR aims to fix #118.

Summary of PR

A new file traverse.js with a function called walk inside it. walk is a utility function that allows a use to walk through the links in a block and, for each one, fetch that block using the load function, which is passed in to walk as a parameter.

The naming is chosen to align with go-ipld-prime.

Test cases have been written, which have led to two new dev dependencies being added in this PR: @ipld/dag-pb and sinon.

src/traverse.js Outdated Show resolved Hide resolved
@patrickwoodhead patrickwoodhead marked this pull request as draft November 19, 2021 10:28
Copy link
Member

@rvagg rvagg 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 also change it from traverse to traversal?
I'm not super jazzed about a new devDependency, sinon is pretty heavy for checking an ordered list of CIDs (and I tend toward minimal dependencies these days, personally!), but that could be refactored out later if need be I suppose.
Looks good otherwise! Though I'm not sure about why check is now complaining about the base exports, I'll have to have a look at that, some new crazy breakage typescript is throwing at us in a non-major release no doubt.

src/traverse.js Outdated Show resolved Hide resolved
src/traverse.js Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Nov 24, 2021

So I tried this out @ ipfs/js-ipfs#3950 and had to make a small modification - the seen optionality needed to be [seen] rather than the ? which implies that it needs to be present & null. I've also rebased this branch and fixed some lint problems and force pushed an update and I think it's good to go. I'll squash when merging.

I think I'm OK with this API. @achingbrain what think you?

@rvagg
Copy link
Member

rvagg commented Nov 24, 2021

Added a thought over @ https://github.com/ipfs/js-ipfs/pull/3950/files#r755899861 that maybe watching for the loader returning null would be a good SkipMe signal, to avoid the need for a faux-block. It'll also serve other cases in future when we do more sophisticated traversals and you want to skip blocks for other reasons.

Thoughts anyone?

src/traversal.js Outdated
* @template T
* @param {Object} options
* @param {CID} options.cid
* @param {(cid: CID) => Promise<Block<T>>} options.load
Copy link
Member

Choose a reason for hiding this comment

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

@patrickwoodhead let's go with the null == skip feature for this.

Suggested change
* @param {(cid: CID) => Promise<Block<T>>} options.load
* @param {(cid: CID) => Promise<Block<T>|null>} options.load

Then you can just do a return if block === null on the load() call.

If you could also add some very basic docs onto the README for this that would be helpful too. As you can see the current README is fairly minimal so no need to go overboard unless you're enthusiastic (it's a TODO #47).

Since I've force pushed a rebase and some additional commits to this branch, you'll have to get your local copy updated. git fetch origin and git rebase origin feat/walk should do the trick, then you can just push new updates.

When we merge, we'll squash it down to a single commit with a feat: prefix so this will be a semver-minor release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have done the above. Let me know if there is anything still outstanding.

src/traversal.js Outdated Show resolved Hide resolved
test/test-traversal.js Outdated Show resolved Hide resolved
test/test-traversal.js Outdated Show resolved Hide resolved
test/test-traversal.js Outdated Show resolved Hide resolved
test/test-traversal.js Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌 superb! great work @patrickwoodhead

test/test-traversal.js Outdated Show resolved Hide resolved
Closes: #118

Basic traversal functionality for deterministic DAG walking with no
repeat block visits and support for block skipping.

User supplies a block loader, which can be used to watch the block
ordering of the walk.
@rvagg
Copy link
Member

rvagg commented Nov 29, 2021

updated ipfs/js-ipfs#3950 to use the newly release 9.5.0, looks good

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Jan 28, 2022
SgtPooki pushed a commit to ipfs/js-kubo-rpc-client that referenced this pull request Aug 18, 2022
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.

TODO: DAG walk for Block API
3 participants