Feat: remove circular dependencies in merkledag package tests #4704
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids using unixfs package and importer packages in merkledag, which removes circular depedencies making it hard to extract this module. License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
ghost
added
the
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
Stebalien
reviewed
Feb 21, 2018
| // First, a node is created from each 512 bytes of data from the reader | ||
| // (like a the Size chunker would do). Then all nodes are added as children | ||
| // to a root node, which is returned. | ||
| func makeTestDAG(t *testing.T, read io.Reader, ds ipld.DAGService) ipld.Node { |
There was a problem hiding this comment.
This should be using io.ReadFull (Read isn't guaranteed to fill the buffer). It should probably also handle short reads. It may currently "work" but I'm worried that users will misuse it.
merkledag/merkledag_test.go
Outdated
| errs <- err | ||
| } | ||
| _ = firstpb | ||
| _ = expected |
There was a problem hiding this comment.
We shouldn't need these lines.
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
Stebalien
requested changes
Feb 27, 2018
merkledag/merkledag_test.go
Outdated
| for err == nil { | ||
| protoNode := NodeWithData(p) | ||
| nodes = append(nodes, protoNode) | ||
| _, err = read.Read(p) |
There was a problem hiding this comment.
Same. Needs a ReadFull. I'd just deduplicate the code and have a for { loop with a break/return.
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
|
Thanks @Stebalien |
Stebalien
approved these changes
Mar 3, 2018
|
Sufficiently paranoid 😄. LGTM. |
|
@hsanjuan would you be offended if I don't merge this one until we ship 0.4.14? |
|
@whyrusleeping when will that be? |
|
@whyrusleeping FYI, this is just a test change. |
Kubuxu
approved these changes
Mar 8, 2018
|
Thanks @hsanjuan! |
ariescodescream
pushed a commit
to ariescodescream/go-ipfs
that referenced
this pull request
Apr 7, 2022
License: MIT Signed-off-by: Hector Sanjuan <hector@protocol.ai>
ariescodescream
pushed a commit
to ariescodescream/go-ipfs
that referenced
this pull request
Apr 7, 2022
Feat: remove circular dependencies in merkledag package tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This avoids using unixfs package and importer packages in
merkledag, which removes circular depedencies making it hard
to extract this module.
This comes pers @whyrusleeping suggestion on IRC:
I'm very new here handling DAGs etc, so excuses in advance if something is terribly stupid.