"Weird" code conventions

[toqueteos]

Things like this are weird for code reviewing.

// Copied from core/core.go

import (
    "fmt"
    ds "github.com/jbenet/datastore.go"
    blocks "github.com/jbenet/go-ipfs/blocks"
    config "github.com/jbenet/go-ipfs/config"
    merkledag "github.com/jbenet/go-ipfs/merkledag"
    path "github.com/jbenet/go-ipfs/path"
    peer "github.com/jbenet/go-ipfs/peer"
)

Most of those named imports are useless (same name as package name) and unsorted.

gofmt will deal with import sorting, separating std from local package and other (non-std) packages would be the best way to go (it doesn't violate any code conventions and it's better for readers/reviewers).

import (
    "fmt"

    "github.com/jbenet/go-ipfs/blocks"
    "github.com/jbenet/go-ipfs/config"
    "github.com/jbenet/go-ipfs/merkledag"
    "github.com/jbenet/go-ipfs/path"
    "github.com/jbenet/go-ipfs/peer"

    ds "github.com/jbenet/datastore.go"
)

[toqueteos]

Most util pkg usage is related to the Key type which I already noted in issues #8 #12 as a bit unnecessary. Here is some constructive criticism to get rid of it.

• POut and PErr log to stdout and stderr (conditionally if util.Debug is true for S variants), this is already handled by std's log pkg. Example:

  // 0 stands for no flags, maybe the date & time would be a good thing for some pkgs,
  // then `log.Ldate|log.Ltime` is enough
  var logger = log.New(os.Stdout, "[pkg] ", 0)

• SOut and Serr do the same as POut and SErr respectively but their output can be negated if util.Debug is true. They are never used, we can get rid of them. Cmd ipfs stdout/err output can always be piped into /dev/null.

• util.Hash (only used in pkgs: blocks and merkledag) should probably live inside pkg core.

• util.TildeExpansion is only used on pkg config, it should live there.

[whyrusleeping]

I hope you realize that there is still a lot of work to be done, things that are "unused" may only be "not used yet". Things that are only used in one place may be used elsewhere later on.

As for the logging, having our own logging is a good thing. yes, go's stdlib logging library does "work", but I have a feeling we will get into some interesting situations where having our own logging wrappers will greatly behoove us.

[toqueteos]

@whyrusleeping Of course, this just started but things tend to be delayed, sometimes forgot. It's better to have a clean codebase from the start and Go makes it easy (go vet, golint and goconvey do help a lot too). As for logging, log will get you a long way, then there's https://github.com/golang/glog.

[jbenet]

@toqueteos thanks for going through and providing constructive feedback.

I disagree with many of the comments. Below is my reasoning. However, I want to express a few high level points:

• In general, I like the idiomatic Go approaches to things. Not only are these recommended by the people who know Go best (the Go team itself), but they tend to make perfect sense.
• I'm still new to Go. My C/C++/Python/JS are all stronger. You can see this with my propensity to overuse pointers in Go, and other things. Please definitely comment on these things.
• There are cases where I strongly disagree with the idiomatic Go approach. I usually have strong reasons for disagreement, and happy to express them.
• My goal is to get something working first, and second to worry about code cleanliness. Of couse, I agree very much with setting these things right from the get go. But there is a fine line between investing in longer term code, and nit picking on code that will be obsolted during develoment anyway. In the many projects I've built, I've found that obsessing with code at first hurts. It's obsessing with the API that is critical. (The code can change easily, the API can't.). While we're still figuring out how all of this is going to work, I expect lots of the code to be written and later thrown out as better ways of doing things come along. bear this in mind :)
• I'm very opinionated. sorry! There's usually good reasons behind it.

---

Most of those named imports are useless (same name as package name) and unsorted.

This is a strong disagreement with idiomatic Go. Imports should be explicit. The package name should be absolutely clear. There is ambiguity when imports like this happen:

import (
  "github.com/jbenet/go-multihash"
)

So as I see it, all package imports should be explicit. (stdlib gets a free ride because I'm not pedantic, rules always break down somewhere)

(it doesn't violate any code conventions and it's better for readers/reviewers).

IMO, visual cleanliness, while important, is trumped by logical cleanliness (imports being explicit). The sorting, placement, etc., I don't care about, sure!

Here is some constructive criticism to get rid of it.

We'll have a LOT of random crap as we go. util is a good placeholder until a better place is found, or a good final place if it really doesn't belong anywhere else and is too small to be its own package.

POut and PErr log to stdout and stderr

For now. This will change, and will only change in one place instead of N (as @whyrusleeping pointed out).

this is already handled by std's log pkg.

Great point, maybe we should switch to using log within {P,D}{Out,Err}

[DOut and DErr] are never used, we can get rid of them

Not true. I use them a lot. I usually remove most debug statements, but some we'll want to start keeping.

Cmd ipfs stdout/err output can always be piped into /dev/null.

Evaluating the print still needs to happen. This might be really bad if printing out entire data structures, which I sometimes do. Hence these functions.

util.Hash (only used in pkgs: blocks and merkledag) should probably live inside pkg core

Cyclic dependency.

util.TildeExpansion is only used on pkg config, it should live there.

Yeah probably right. Feel free to move it. It might get used elsewhere, and then we can move it back.

This, of course, is unnecessary work at this time.

As for logging, log will get you a long way, then there's https://github.com/golang/glog.

Looks interesting! Might be overkill atm, but probably the right way to go later, thanks!!

@whyrusleeping has precisely the right perspective here:

I hope you realize that there is still a lot of work to be done, things that are "unused" may only be "not used yet". Things that are only used in one place may be used elsewhere later on.

Thanks for the effort + feedback, @toqueteos! Very much appreciated. Just please bear in mind my perspective here, and priorities. The goal is to move fast until a stable first implementation exists.

By the way, i'd like to point out that lots of git is written in shell scripts. This makes me cringe. But I totally get why they did that. It made it much faster for them to iterate, etc. likewise here, I'm not going to be a stickler for codebase style rules or implementations being precisely how i think they should work. Rather, the main concern is getting the API right, and working, robust implementation.

Thanks!! :)

[toqueteos]

You are welcome @jbenet! Your repo, your project, your rules but be aware that more Go style criticism will appear as time goes and ipfs gets more traction (which it deserves, lots of it).

All of this was found while I was reading the code and checking the whitepaper, checking what is implemented and what not trying to understand how all the ideas fit together and even maybe implement some pieces myself.

---

Just a quick reply to some of your answers:

1. Imports.

   import blocks "github.com/jbenet/go-ipfs/blocks"

   This approach is redundant and doesn't clarify nothing, because package names usually have the same name as the last part of their path. Sources: http://golang.org/doc/code.html#PackageNames and http://golang.org/doc/effective_go.html#names.

   Maybe pkg github.com/jbenet/go-multihash has to be renamed to just github.com/jbenet/multihash but that may be a problem for you because you implemented multihash in other languages.

   Same for pkg github.com/jbenet/datastore.go, which, if I recall correctly may cause problems with go get because of its URL ending in .go. I need to confirm this, most github.com/user/project.go moved recently to just github.com/user/project because of issues with that.

   Although this is totally fine:

   import mh "github.com/jbenet/go-multihash" // package name is multihash referenced as mh

   Up to you.

2. util.Hash and core pkg. The Hash fn can't be moved into core because of import cycles but right now pkg core doesn't really have the core of ipfs, it just defines the IpfsNode interface and two datastore private helpers.

---

I won't bitch anymore about conventions.

[jbenet]

You are welcome @jbenet! Your repo, your project, your rules but be aware that more Go style criticism will appear as time goes and ipfs gets more traction (which it deserves, lots of it).

Yep! Understood, and criticism is definitely welcome. I just won't always change my mind ;). And thanks!! :D

This approach is redundant and doesn't clarify nothing, because package names usually have the same name as the last part of their path.

This is not always the case. If I go and create a new version that happens to change the package name I now have to rewrite code, not just update an import. This is IMO a failure in the go design.

Maybe pkg github.com/jbenet/go-multihash has to be renamed to just github.com/jbenet/multihash but that may be a problem for you because you implemented multihash in other languages.

The fact that the logical naming may or may not (but not always does or not) match the package import addresses reflects how bad of a design idea this was. And the fact that you can't fix to particular versions, e.g.

import (
  multihash "github.com/jbenet/go-multihash@deadbeef"
)

(without making them accessible in directories at the endpoint) makes me sad.

most github.com/user/project.go moved recently to just github.com/user/project because of issues with that.

Since you brought it up, I'll rant a little bit about this. :)

This is so stupid. REQUIRING a change in the names that projects get in websites external to Go is simply absurd. Implementations in multiple languages happen. It is standard to prefix or suffix languages in the repo names to clarify. This shouldn't be a problem. And this is just one of many reasons. The Go team + community should:

• build diamond dep friendly versioning support into go get,
• make all imports logically explicit, and maybe
• finally build a package manager to ensure all library versions are available in the future.

The Hash fn can't be moved into core because of import cycles but right now pkg core doesn't really have the core of ipfs,

Ah, but it will :). See https://github.com/jbenet/node-ipfs/blob/master/submodules/ipfs-core/index.js

---

Thanks!

[jbenet]

Oh will add that, Go team was smart and didn't force this at all. It's bad that it's idiomatic convention though. So, my imports are explicit. And once IPFS is up, versioned correctly. Soon, we'll have things like:

import (
    ipfs "/ipns/gopkg.io/ipfs/1.2.3"
    mh "/ipns/gopkg.io/multihash/deadbeef1234567890"
    ma "/ipfs/deadbeef1234567890/go-multiaddr"
)

More at jbenet/random-ideas#19