Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Separation of core and helper methods #8

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Conversation

acruikshank
Copy link
Contributor

@acruikshank acruikshank commented Mar 1, 2021

closes #7

Motivation

The fetcher adds many functions to fetch IPLD prime through IPFS. Most of these methods are simply conveniences to provide a simple interface for common tasks. This proliferation of methods will complicate future efforts to extend or replace this library. A better pattern is to provide a few very flexible methods to provide core functionality (fetcherSession), give it an interface, then call the rest helper methods and modify them to use this interface.

Note: The issue discussed limiting the core to BlockMatchingOfType and NodeMatching. Arguably, BlockOfType is more core than BlockMatchingOfType and in practice either of these requires significant additional complexity to implement from the other (I tried it both ways). Consequently, I've left both BlockOfType and BlockMatchingOfType in the core.

What's in this PR

  1. Add Fetcher interface for core methods, and rename Fetcher struct to fetcherSession.
  2. Modify all methods outside core to be free functions that take a Fetcher as a parameter.
  3. Add tests for all functions not already covered.
  4. After comments from @warpfork and the discovery of a race condition in the tests, convert the return from 2 channels to 1 channel with an error field in the result.
  5. Align licensing with the go-ipfs project (MIT+Apache2).

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

Nice! This seems like a pretty clean interface

README.md Outdated Show resolved Hide resolved
fetcher.go Show resolved Hide resolved
fetcher.go Outdated
Comment on lines 35 to 41
// crossing block boundaries. The nodes will be typed using the provided prototype. Each matched node is sent to
// the FetchResult channel.
BlockMatchingOfType(ctx context.Context, root ipld.Link, match selector.Selector, ptype ipld.NodePrototype) (<-chan FetchResult, <-chan error)
Copy link
Member

Choose a reason for hiding this comment

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

Should (both of) the result channels be expected to be closed when this request is done?

Will the error channel be able to yield more than one error? Can FetchResult and errors be yielded intermingled?

Would this perhaps be clearer if this was unified into one channel, and FetchResult just contained either a Result or an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're last comment is correct. I gave to 2 channels a go, but I think it's just too complicated to explain the behavior and properly process the results. I've changed it to a single channel with an error in the result.

Copy link
Contributor Author

@acruikshank acruikshank Mar 2, 2021

Choose a reason for hiding this comment

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

Strike that. @hannahhoward pointed out ipfs/interface-go-ipfs-core#62. The reasoning here makes sense, so I'm keeping the error channel, modifying it so that it's always buffered and closed on completion and then documenting the behavior in the interface.

@acruikshank acruikshank force-pushed the feat/common_and_helpers branch 2 times, most recently from 965ad1c to 44f845d Compare March 2, 2021 17:29
@acruikshank acruikshank merged commit 7229dd1 into main Mar 2, 2021
Jorropo pushed a commit to ipfs/go-libipfs-rapide that referenced this pull request Mar 23, 2023
Separation of core and helper methods and better channel behavior

This commit was moved from ipfs/go-fetcher@7229dd1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider separation between 'core interface' and helper methods
3 participants