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

Golint: fix golint warnings in merkledag submodule #4665

Merged
merged 4 commits into from
Feb 7, 2018

Conversation

hsanjuan
Copy link
Contributor

@hsanjuan hsanjuan commented Feb 6, 2018

This fixes all golint warnings in merkledag (except in protobuf generated code).

Main change of note is that NewDAGService returns an ipld.DAGService and not a non-exported dagService.

@ghost ghost assigned hsanjuan Feb 6, 2018
@ghost ghost added the status/in-progress In progress label Feb 6, 2018
License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
// NewDAGService constructs a new DAGService (using the default implementation).
func NewDAGService(bs bserv.BlockService) *dagService {
Copy link
Member

Choose a reason for hiding this comment

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

Does golint really complain about this?

I actually prefer to return concrete types whenever possible. It helps to deal with multiple potentially fulfilled interfaces. For example, if *dagService implements X which is a superset of DAGService, I can pass NewDAGService to something that wants an X without extra type assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want me to export dagService instead?

@@ -241,9 +242,10 @@ func TestFetchGraph(t *testing.T) {
// create an offline dagstore and ensure all blocks were fetched
bs := bserv.New(bsis[1].Blockstore(), offline.Exchange(bsis[1].Blockstore()))

offline_ds := NewDAGService(bs)
// we know the default dagService implements LinkGetter
offlineDS := NewDAGService(bs).(ipld.LinkGetter)
Copy link
Member

Choose a reason for hiding this comment

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

This is exactly the case for having NewDAGService return the concrete type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is a case only used in tests (and tests in the same module). From an external user's point of view, it's not possible to know what *dagService can do because it's unexported.

@whyrusleeping
Copy link
Member

Thank you!!

Everything except the DAGService return type LGTM... If golint really complains about that it makes me a tad bit sad.

@kevina
Copy link
Contributor

kevina commented Feb 6, 2018

Everything except the DAGService return type LGTM... If golint really complains about that it makes me a tad bit sad.

I agree and I want to emphasis that we don't have to follow all off golints suggestions. If this will cause codeclimate to complain we should add exception.

@Stebalien
Copy link
Member

Everything except the DAGService return type LGTM... If golint really complains about that it makes me a tad bit sad.

It does this for a (possibly debatable) reason. The lint disallows returning unexported types because they can't be named outside the package.

@hsanjuan
Copy link
Contributor Author

hsanjuan commented Feb 6, 2018

Ok, I have reverted to what it was before and get the expected:

merkledag.go:33:43: exported func NewDAGService returns unexported type *merkledag.dagService, which can be annoying to use

If it is valuable that this is used as a LinkGetter (which is not except in tests), I would lean to exporting it though. The user won't have a clue that this returns a LinkGetter without looking at sources otherwise. It's a nuance anyway, I'm fine with whatever.

@@ -14,28 +14,34 @@ type ErrorService struct {

var _ ipld.DAGService = (*ErrorService)(nil)

// Add returns an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this description confusing, rather than say "an error" something like "returns ErrorService.Err" or maybe just "returns the error" rather than "an error".

func (cs *ErrorService) Add(ctx context.Context, nd ipld.Node) error {
return cs.Err
}

// AddMany returns an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

func (cs *ErrorService) AddMany(ctx context.Context, nds []ipld.Node) error {
return cs.Err
}

// Get returns an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Etc.

@@ -23,7 +23,13 @@ func init() {
ipld.Register(cid.DagCBOR, ipldcbor.DecodeBlock)
}

// contextKey is a type to use as value for the ProgressTracker contexts.
type contextKey string
Copy link
Contributor

Choose a reason for hiding this comment

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

I would export this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has no use outside. If someone ever has a use, it can be exported then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, your right. Sorry.

Per @kevina's comments

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@Stebalien
Copy link
Member

FYI,

If it is valuable that this is used as a LinkGetter (which is not except in tests), I would lean to exporting it though. The user won't have a clue that this returns a LinkGetter without looking at sources otherwise. It's a nuance anyway, I'm fine with whatever.

In general, users shouldn't care if something is a LinkGetter. They should just call ipld.GetLinks(ctx, dag, cid) which will, internally, try to cast dag to a LinkGetter if the passed dag implements that interface.

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.

One import nit. Otherwise, LGTM. ❤️

@@ -13,6 +13,8 @@ import (
"testing"
"time"

blocks "gx/ipfs/Qmej7nf81hi2x2tvjRBF3mcp74sQyuDH4VMYDGd1YtXjb2/go-block-format"
Copy link
Member

Choose a reason for hiding this comment

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

This should be grouped with cid, u, etc. (we group by standard, in-repo, then out-of-repo).

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@hsanjuan
Copy link
Contributor Author

hsanjuan commented Feb 7, 2018

opps. fixed @Stebalien

@whyrusleeping whyrusleeping merged commit 12354cc into master Feb 7, 2018
@ghost ghost removed the status/in-progress In progress label Feb 7, 2018
@whyrusleeping whyrusleeping deleted the doc/golint-merkledag branch February 7, 2018 20:40
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.

4 participants