add in a default file hash and cleaned up init function a bit

[whyrusleeping]

No description provided.

[whyrusleeping]

So, are we good to close this PR then? if the changes have been applied to the new commands?

[btc]

does the node need to be closed everywhere it is instantiated?

[whyrusleeping]

I would feel better if it was, technically it doesnt need to be, because in most of our uses, no longer needing the node means closing down the program and letting the operating system clean up, but Im always slightly paranoid

[btc]

i see. we may know better, but we can't rely on our individual knowledge. developers who import our library and instantiate nodes programmatically may have other plans. the system needs to speak for itself.

perhaps we should communicate this through the function signature:

node, cancel, err := core.Node(...)

where the implementation does the following:

ctx, cancel := ...

... create ...

return node, cancel, nil

[jbenet]

would feel better if it was

agreed.

node, cancel, err := core.Node(...)

👎 leaking underlying libs and abstractions is not clean. This is what the Closer interface is for.

node, err :=  ipfs.NewNode(...)
defer node.Close()
...

Internally, that Close method can do whatever, but our interfaces do not leak out the entrails of our implementations.

[btc]

That's incorrect. cancel is a func(). That is not leaking an abstraction. It's explanatory, and forces the caller to make a conscious decision to ignore shutdown. Much safer practice than depending on the programmer to call defer Close() at all call sites.

Design the system to minimize programmer error. This only increases in importance as the system grows.

Good interfaces are:
Easy to use correctly. People using a well-designed interface almost always use the interface correctly, because that's the path of least resistance. In an API, they almost always pass the correct parameters with the correct values, because that's what's most natural. With interfaces that are easy to use correctly, things just work.
Hard to use incorrectly. Good interfaces anticipate mistakes people might make and make them difficult — ideally impossible — to commit.

people might forget to close => make it impossible to forget

We have empirical evidence that the existing interface is easy to use incorrectly.

[whyrusleeping]

ehh.... im still not super sold on the returning a cancel func idea... It doesnt feel like idiomatic Go to me.

[btc]

Fair. I stand by merits of the approach, but accept and acknowledge its downsides. I'm not sold either. I'm okay with continuing to manage the node the same way we do files and locks.

[whyrusleeping]

In a way, its the same as making people remember to close a file, or a socket

[jbenet]

forces the caller to make a conscious decision to ignore shutdown. Much safer practice than depending on the programmer to call defer Close() at all call sites.

I agree that is a nice property. An approach like:

unlock := lock.Lock()
doSomethingCritical()
unlock()

Might avoid making mistakes a bit more. But note it doesn't actually force the user to deal with the return value.

lock.Lock()
doSomethingCritical
// oops

doesn't even get a warning. So it's not an 100% solution.

---

The added complexity and leaking of abstractions is that if cancel its not going to be called in the same function than you have to continue leaking it out, or add it to a new object:

// you have to carry around an additional cancel _everywhere_ you pass n.
// now prone to programming and design error.
// the function is now state you're passing around. hiding it in a callstack does not change that.
func constructNode() ipfs.IPFS, context.CancelFunc {
  cfg := ....
  ...
  ...
  n, cancel := ipfs.NewNode(...)
  ...
  ...
  ...
  return n, cancel 
}

// or what will invariably happen:

type nodeCtx struct {
  node ipfs.IPFS
  cancel context.CancelFunc
}


func constructNode() nodeCtx {
  cfg := ....
  ...
  ...
  n, cancel := ipfs.NewNode(...)
  ...
  ...
  ...
  return nodeCtx{node: n, cancel: cancel}
}

I dont think passing these cancel functions around is a good idea. I'd be curious to see what go-nuts have to say about it.

---

This is not critical. Let's keep it in mind and move on.

[btc]

Agreed.

[btc]

LGTM

[whyrusleeping]

@jbenet @maybebtc updated this and ported the changes to ipfs2, can i get a tiny bit more CR?

[jbenet]

though init is local, we've moved to not have any Printf anywhere except main. Return the string and have it printed as usual cmd output

[jbenet]

nevermind, init already prints this way... >.<

[jbenet]

i'm going to merge this in-- now that travis is finally done.

[whyrusleeping]

👏 👍