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

add tests, make golint pass #29

Closed
wants to merge 2 commits into from
Closed

add tests, make golint pass #29

wants to merge 2 commits into from

Conversation

b5
Copy link

@b5 b5 commented Nov 30, 2017

might as well write tests while learning stuff. I've also added comments to make golint happy, feel free to let me know if any of those are inaccurate.

cc @Stebalien

Copy link
Member

@Stebalien Stebalien 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 taking the time to put this together. I'd actually take a look at #8 (and possibly make the pull request against that branch) as it contains many interface changes and some new documentation.

}

// DefaultBlockDecoder is a safeBlockDecoder with no decoders registered
Copy link
Member

Choose a reason for hiding this comment

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

safeBlockDecoder isn't exported so I wouldn't mention it by here. I'd just say "DefaultBlockDecoder is the block decoder used by Decode" and maybe mention that (a) it's threadsafe and (b) users should register block decoders with it.

@@ -9,6 +9,7 @@ import (
cid "github.com/ipfs/go-cid"
)

// Resolver is the interface that details paths a node contains
Copy link
Member

Choose a reason for hiding this comment

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

Resolver is more something that can be resolved through than something that "details" (although I'm planning on reworking these interfaces in the near future).

@@ -19,6 +20,7 @@ type Resolver interface {
Tree(path string, depth int) []string
}

// Node is an object in a Merkle DAG graph
Copy link
Member

Choose a reason for hiding this comment

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

  • DAG graph is redundant.
  • We usually say merkledag as one word.

@@ -40,7 +42,9 @@ type Node interface {
Size() (uint64, error)
}

// NodeGetter is the interface that wraps the basic get method
Copy link
Member

Choose a reason for hiding this comment

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

NodeGetter is really just a read-only DAG service. I'd take a look at #8, it makes a lot of this clearer. Actually, I recommend making the pull request against that branch as I'll likely merge that first.

@@ -56,6 +60,11 @@ type Link struct {
Cid *cid.Cid
}

// GetNode returns the MDAG Node that this link points to
Copy link
Member

Choose a reason for hiding this comment

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

IPLD Node.

@b5
Copy link
Author

b5 commented Nov 30, 2017

Oooooh ok makes sense. Well at least I still learned a thing :)

@b5 b5 closed this Nov 30, 2017
@Stebalien
Copy link
Member

Also, documentation is always welcome.

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