Refactor feat/immutable PR #7

haadcode · 2016-11-14T15:24:51Z

Refactored #6

haadcode · 2016-11-14T15:25:35Z

This PR doesn't pass tests yet, it's WIP but wanted to start the discussion.

daviddias · 2016-11-14T15:26:29Z

src/dag-link.js

      name: this.name,
      size: this.size,
-      hash: this.multihash ? mh.toB58String(this.multihash) : undefined
+      hash: this.multihash ? mh.toB58String(this.multihash) : null
    }
  }


haadcode · 2016-11-14T15:26:36Z

There's also couple of places where the code needs to handle callback vs. Promises, mainly the DAGNodeFactory methods like create(). Will update/fix that soon.

daviddias · 2016-11-14T15:26:53Z

src/dag-link.js

+    return this._multihash ? mh.toB58String(this._multihash) : undefined
+  }
+
+  set hash() {


s/hash/multihash

daviddias · 2016-11-14T15:27:00Z

src/dag-link.js

+    throw ImmutableError
+  }
+
+  get hash() {


s/hash/multihash

haadcode · 2016-11-14T15:27:15Z

Let me know if you want to hear the reasoning behind the changes.

daviddias · 2016-11-14T15:27:36Z

src/dag-node-factory.js

+const DAGLink = require('./dag-link')
+
+class DAGNodeFactory {
+  static create (data, dagLinks, hashAlg) {


isn't this the same as exporting a function called create?

Also, I can see the value of having the create function in a separate file, having to do: dagPB.dagNodeFactory.create(..), seems a tad long, just to create an instance, we can scope the create function withing dagPB.DAGNode even if it is in a separate file.

daviddias · 2016-11-14T15:30:13Z

src/dag-node-factory.js

+        return l
+      } 
+
+      // haadcode: are the .name vs .Name for backwards compatibility?


yep (go-ipfs outputs json with that)

daviddias · 2016-11-14T15:31:26Z

src/dag-node-factory.js

+    })
+
+    // haadcode: would be better to have: links = sort(links, util.linkSort)
+    sort(links, util.linkSort)


the sort is a sort inplace, which works well with a immutable property

daviddias · 2016-11-14T15:33:36Z

src/dag-node-factory.js

+          .then((serialized) => util.hash(hashAlg, serialized))
+          .then((multihash) => new DAGNode(data, links, serialized, multihash))
+      */
+    })


I don't believe we are going to change this internal component to a Promised based one now. As we discussed in the past and again at the last gathering, so far the guideline is callbacks at the internals, promises+callbacks at the top level.

This will only change if there is a huge discussion and decision here: ipfs/js-ipfs#557

daviddias · 2016-11-14T15:35:12Z

src/dag-node-factory.js

+    }
+
+    // haadcode: shouldn't we clone the serialized and multihash properties too?
+    return DAGNodeFactory.create(data, links, dagNode.serialized, dagNode.multihash)


.create should not take the serialized and multihash, that is generated inside create (or it was in the main PR), note that if you add a link, then the serialized and multihash is different, this is probably why this PR is failing the tests.

daviddias · 2016-11-14T15:35:26Z

src/dag-node-factory.js

+    }
+
+    // haadcode: shouldn't we clone the serialized and multihash properties too?
+    return DAGNodeFactory.create(data, links, dagNode.serialized, dagNode.multihash)


same as https://github.com/ipld/js-ipld-dag-pb/pull/7/files#r87823980

daviddias · 2016-11-14T15:35:55Z

src/dag-node-factory.js

+    const data = DAGNodeFactory._cloneData(dagNode)
+    const links = DAGNodeFactory._cloneLinks(dagNode)
+    // haadcode: shouldn't we clone the serialized and multihash properties too?
+    return DAGNodeFactory.create(data, links, dagNode.serialized, dagNode.multihash)


same as https://github.com/ipld/js-ipld-dag-pb/pull/7/files#r87823980

daviddias · 2016-11-14T15:36:31Z

src/dag-node.js


 class DAGNode {
-  constructor (data, links, serialized, size, multihash, json) {
+  constructor (data, links, serialized, multihash) {
+    // Should these be "throw new Error()" instead?


that is what assert do if it is missing

daviddias · 2016-11-14T15:37:42Z

src/dag-node.js

+      data: this.data,
+      links: this.links.map((l) => l.json),
+      hash: mh.toB58String(this.multihash),
+      size: this.size


this size is incorrect, size of a dagnode is the length of its serialized version + the cumulative length of all its links

Note that the size comes from this.size:

get size () { return this.links.reduce((sum, l) => sum + l.size, this.serialized.length) }

Since it is immutable, we don't need to do this ops everytime, just on creation

You're right. Same for the other toJSON() props. Will change it so that they all get calculated in the constructor. A small speed improvement is a big speed improvement :)

daviddias · 2016-11-14T15:38:16Z

src/index.js

@@ -1,6 +1,6 @@
 'use strict'

-exports.DAGNode = require('./dag-node.js')
+exports.DAGNode = require('./dag-node-factory.js')


we need to expose the class too.

We can't expose the DAGNode class. If we do that, DAGNode will become mutable, ie. doing const node = new DAGNode(...) becomes possible which is not what we want.

The general idea of the factory pattern is to expose only methods for creating the DAGNodes, not for users to directly use DAGNode as it was before. This is the first step to immutability.

We can force the users to always use the 'create', but exposing the Class does not make any instance mutable though.

If we expose the DAGNode class, it'll be possible for users to use it, ie. use the constructor. The only way to force not to do that is to not expose it and that, like said, is the first step to an immutable data structure. This is a generally known and widely-used pattern: don't expose the data structure (for use) but rather provide an interface that returns the data structure (DAGNode in our case).

Is there a reason why we would need to expose it?

sounds good

haadcode · 2016-11-14T15:45:20Z

Will address the feedback, push and come back to the comments

daviddias · 2016-11-14T15:47:48Z

Thank you for giving a thorough review and coming up with a new structure. I've made a fair amount of comments, it would be ideal if tests were passing so that we know the PR is correct (you will see comments showing on why it's not).

Things I like and will certainly adopt for #6:

Avoiding Object.defineProperty, as also mentioned by @dignifiedquire (feat: update DAGLink and DAGNode to have an immutable API #6 (comment))
Factoring out the create function, so that the DAGNode file is less long
Make .json a .toJSON sync function

Things I'm not in favour and given our previous discussions, they won't be included in the main PR

Migration to Promises for internals as noted and justified at : Refactor feat/immutable PR #7 (comment)
- Note: It actualy might make sense to support also a promises API for the .create function since it is a top level consumed object (from ipfs.object.get), so that people don't have their promise chain broken, however, we are not dropping callbacks.

Things that don't make sense to me to have throughout our codebase:

Define "Classes with static methods just to export a bunch of functions" instead of exporting functions with the conventional module.exports. It is mostly a style/convention change which doesn't add any benefit.

daviddias

Needs review: #7 (comment)

haadcode · 2016-11-14T16:11:20Z

The PR now passes tests. Linter is happy except one (too many nested callbacks in tests), which was there from the original PR.

haadcode · 2016-11-14T16:18:13Z

Migration to Promises for internals as noted and justified at : #7 (comment)
Note: It actualy might make sense to support also a promises API for the .create function since it is a top level consumed object (from ipfs.object.get), so that people don't have their promise chain broken, however, we are not dropping callbacks.

I removed the Promises from all the methods. I do think we should expose them with Promises, too, as we do in all our public facing APIs. I assume users will need to create DAGNodes themselves (ie. not used just internally), right?

Define "Classes with static methods just to export a bunch of functions" instead of exporting functions with the conventional module.exports. It is mostly a style/convention change which doesn't add any benefit.

This is matter of a) preference and b) code style. If we use classes, we should use them consistently. If we use functions and prototypical inheritance, then use that consistently across the project. As for preference, I personally think wrapping the functions to static methods in a class is much more clearer than exporting functions (looks cleaner and makes the namespacing intuitive).

haadcode · 2016-11-14T16:19:05Z

I also updated the README to reflect how the DAGNode would be used with the changes in this PR but in short:

dagPB.DAGNode.create(..., cb)

It's the same as in the original PR.

daviddias · 2016-11-14T16:43:58Z

Rad! We are converging! Made a couple of comments :)

haadcode · 2016-11-15T07:49:51Z

@diasdavid what do you think about changing dag-node-factory.js to dag-node-utils.js and DAGNodeFactory to DAGNodeUtils? I originally named it Fatory thinking it'll only create DAGNodes but since we have all the utils (addLink, etc.) in there, I think DAGNodeUtils would make more sense.

daviddias · 2016-11-16T09:27:00Z

src/dag-node-factory.js

+const DAGNode = require('./dag-node')
+const DAGLink = require('./dag-link')
+
+class DAGNodeFactory {


Let's move this to

module.exports = create function create ...

daviddias · 2016-11-16T09:27:16Z

src/dag-node-factory.js

+const DAGLink = require('./dag-link')
+
+class DAGNodeFactory {
+  static create (data, dagLinks, hashAlg, callback) {


wrap the create in promisify, so that it supports both.
plus add a test for promises

to ref: #7 (comment)

Note: It actualy might make sense to support also a promises API for the .create function since it is a top level consumed object (from ipfs.object.get), so that people don't have their promise chain broken, however, we are not dropping callbacks.

daviddias · 2016-11-16T09:27:42Z

src/dag-node-factory.js

+      dagLinks = []
+    }
+    if (typeof hashAlg === 'function') {
+      // empty obj


what does this comment mean?

daviddias · 2016-11-16T09:27:45Z

src/dag-node-factory.js

+      data = undefined
+    }
+    if (typeof dagLinks === 'function') {
+      // empty obj


what does this comment mean?

I don't know, thought you left them there originally? :)

daviddias · 2016-11-16T09:30:40Z

src/dag-node-factory.js

+    }
+
+    DAGNodeFactory.create(data, links, callback)
+  }


Not sure if we want this methods in a thing that is called factory.

Not only forces people to have to require the module if they want to add a link:

const node = <some dagnode I got from ipfs.object.get // now I want to add a link const dagNodeFactory = require('ipld-dag-pb').dagNodeFactory dagNodeFactory.addLink(node...)

while it could just be (as it was):

const newNode = node.addLink(link)

Almost, it would be used like this:

const node = <some dagnode I got from ipfs.object.get // now I want to add a link const DAGNode = require('ipld-dag-pb').DAGNode DAGNode.addLink(node...)

This is a general functional pattern whereas const newNode = node.addLink(link) is really neither. If you have an instance node and you call a method on it, that implies the state of the node changes (not that it returns a new node).

daviddias · 2016-11-16T09:30:55Z

src/dag-node-factory.js

+    DAGNodeFactory.create(data, links, callback)
+  }
+
+  static removeLink (dagNode, nameOrMultihash, callback) {


same as addLink

daviddias · 2016-11-16T09:32:21Z

src/dag-node.js

+      data: this.data,
+      links: this.links.map((l) => l.json),
+      hash: mh.toB58String(this.multihash),
+      size: this.size


This makes it so that folks can change the values by doing node._data = something while in the previous case that would not happen. Are we happy with that?

While that is true, I think documenting what IS available and prefixing members with "_" is a strong-enough indication how to use it.

If people want to shoot themselves in the foot they will find a way to do so ;)

sounds good :)

daviddias · 2016-11-16T09:33:09Z

src/dag-node.js


-    stable.inplace(links, util.linkSort)
+  get size () {
+    return this.links.reduce((sum, l) => sum + l.size, this.serialized.length)


This is still getting calculated every single time size is requested, if a node has 100 links, this starts getting expensive.

Pushed a change for this.

daviddias · 2016-11-16T09:36:42Z

src/index.js

@@ -1,6 +1,6 @@
 'use strict'

-exports.DAGNode = require('./dag-node.js')
+exports.DAGNode = require('./dag-node-factory.js')


sounds good

daviddias · 2016-11-16T09:38:58Z

test/dag-node-test.js

@@ -29,10 +29,10 @@ module.exports = (repo) => {
        expect(Buffer.isBuffer(node.data)).to.be.true.mark()
        expect(node.size).to.be.above(0).mark()

-        util.serialize(node, (err, serialized) => {
+        DAGNode._serialize(node, (err, serialized) => {


the serialize and deserialize need to remain at the util package, it is the definition of the IPLD Format

https://github.com/ipld/interface-ipld-format#ipld-format-utils

This doesn't make sense. Why would the serialization functionality be separate from all other functionality of a specific data structure (DAGNode)?

Also, not possible atm as deserialize uses .create() (two-way coupling between utils and dag-node-utils).

Take that back, I understand what you mean with the comment. However, the util API doesn't make sense to me. Why would they need to be namespaced under utils instead of haing them on the top level? Ie.

const DAGNode = require('dag-node') const node = DAGNode.create(...) DAGNode.serialize(node, ...) DAGNode.deserialize(node, ...) // etc.

I can move the utils to one place, but my questions still stands: why do we need utils to be part of the API?

Ignore all this, I'm not gonna bikeshed this now. I'll make some changes and push.

@haadcode it does make sense because util and resolver is what an IPLD Resolver needs in order to process an IPLD Format, this is generic to any API resolver.

Why does IPLD need util?

this is generic to any API resolver.

What does that mean?

dignifiedquire · 2016-11-16T10:30:35Z

package.json

@@ -61,6 +61,7 @@
    "lodash": "^4.17.0",
    "ncp": "^2.0.0",
    "pre-commit": "^1.1.3",
-    "rimraf": "^2.5.4"
+    "rimraf": "^2.5.4",
+    "webpack": "2.1.0-beta.25"


not needed anymore

latest aegir fixes the issues, and webpack shouldn't be explicitly depended on anyway as it is brought in by aegir.

same as libp2p/js-libp2p-floodsub#9 (comment)

Installing with latest aegir still has the same problem as before: browser tests don't run. The error is

Uncaught Error: Module '/Users/samuli/code/js-ipld-dag-pb/node_modules/buffer/index.js' is not a loader (must have normal or pitch function) at loadLoader (node_modules/loader-runner/lib/loadLoader.js:35:10) at iteratePitchingLoaders (node_modules/loader-runner/lib/LoaderRunner.js:164:2) at runLoaders (node_modules/loader-runner/lib/LoaderRunner.js:357:2) at NormalModule.doBuild (node_modules/webpack/lib/NormalModule.js:131:2) at NormalModule.build (node_modules/webpack/lib/NormalModule.js:179:15) at Compilation.buildModule (node_modules/webpack/lib/Compilation.js:127:9) at node_modules/webpack/lib/Compilation.js:306:10 at node_modules/webpack/lib/NormalModuleFactory.js:74:13 at NormalModuleFactory.applyPluginsAsyncWaterfall (node_modules/tapable/lib/Tapable.js:123:70) at onDoneResolving (node_modules/webpack/lib/NormalModuleFactory.js:49:11) at onDoneResolving (node_modules/webpack/lib/NormalModuleFactory.js:165:6) at node_modules/webpack/lib/NormalModuleFactory.js:161:6 at node_modules/async/dist/async.js:3853:9 at node_modules/async/dist/async.js:484:16 at iteratorCallback (node_modules/async/dist/async.js:1084:13) at node_modules/async/dist/async.js:988:16

dignifiedquire · 2016-11-16T10:31:57Z

src/dag-link.js

@@ -2,28 +2,57 @@

 const mh = require('multihashes')

+const ImmutableError = new Error('Immutable property')


would be nicer to extend Error and use like this

class ImmutableError extends Error {} throw new ImmutableError('Size is immutable')

Refactored the errors to be specific now.

dignifiedquire · 2016-11-16T10:42:22Z

src/dag-link.js

+  }
+
+  set multihash (multihash) {
+    throw ImmutableError


this is not good, if you don't use new here it will have an incorrect stacktrace I think

@dignifiedquire not sure if it applies to errors in the same way, but yeah, your suggestion above -- #7 (comment) -- makes sense.

dignifiedquire · 2016-11-16T10:43:27Z

src/dag-node-utils.js

+exports.addLink = addLink
+exports.removeLink = removeLink
+exports.toDAGLink = toDAGLink
+exports.util = {


why are these namespaced and the others aren't?

See #7 (review)

because of IPLD Format definition looking for the util

daviddias · 2016-11-16T10:46:22Z

src/dag-node-utils.js

+}
+
+exports.create = create
+exports.clone = clone


why did clone get separated from the class node? const copy = node.clone() was nice

Same as for node.addLink. Let's either go with functional style, or OO style. Let's not mix the two.

sounds good

daviddias · 2016-11-16T10:46:43Z

src/dag-node-utils.js

+
+exports.create = create
+exports.clone = clone
+exports.addLink = addLink


still not convinced that these methods should not be at at the node level

Of course they should. These are all functions that one can do to create and manipulate a DAGNode.

ok, maybe I am, this answer was hidden #7 (comment)

daviddias · 2016-11-16T10:50:37Z

README.md

@@ -50,9 +49,11 @@ dagPB.util
 Create a new DAGNode

 ```JavaScript
-var node = new dagPB.DAGNode([<data>, <[links]>])
+dagPB.DAGNode.create([<data>, <[links]>, hashAlgorithm, callback]) 


needs to be updated

still needs to be updated

What needs to be updated here? This is the function signature.

daviddias · 2016-11-16T10:50:49Z

README.md

 ```

+#### TODO: update the functions docs from here on out
+


needs to be done

still needs to be updated

daviddias · 2016-11-16T10:51:00Z

package.json

@@ -61,6 +61,7 @@
    "lodash": "^4.17.0",
    "ncp": "^2.0.0",
    "pre-commit": "^1.1.3",
-    "rimraf": "^2.5.4"
+    "rimraf": "^2.5.4",
+    "webpack": "2.1.0-beta.25"


same as libp2p/js-libp2p-floodsub#9 (comment)

daviddias · 2016-11-16T10:51:42Z

src/dag-link.js

      name: this.name,
      size: this.size,
-      hash: this.multihash ? mh.toB58String(this.multihash) : undefined
+      hash: this.multihash ? mh.toB58String(this.multihash) : null


it is the same thing

No, they're not. But let's not get into that now, will change it back to undefined.

daviddias · 2016-11-16T10:52:03Z

src/dag-link.js

+  }
+
+  set multihash (multihash) {
+    throw new Error(`Can't set property: 'multihash' is immutable`)


can we get that "ImmutableError" here too?

Wrapping them to ImmutableError doesn't make sense anymore. What we would need to do is:

class ImmutableError extends Error { constructor(error) { super(error) } }

So let's just throw an Error.

daviddias · 2016-11-16T10:56:28Z

src/dag-node-utils.js

+}
+
+function removeLink (dagNode, nameOrMultihash, callback) {
+  let data = _cloneData(dagNode)


can be a const

daviddias · 2016-11-16T10:56:46Z

src/dag-node-utils.js

+
+function removeLink (dagNode, nameOrMultihash, callback) {
+  let data = _cloneData(dagNode)
+  let links = _cloneLinks(dagNode)


can be const

daviddias · 2016-11-16T10:57:13Z

src/dag-node-utils.js

+  } else if (Buffer.isBuffer(nameOrMultihash)) {
+    links = dagNode.links.filter((link) => !link.hash.equals(nameOrMultihash))
+  } else {
+    throw new Error('second arg needs to be a name or multihash')


if the function uses callbacks, errors should go in callbacks

daviddias · 2016-11-16T10:58:39Z

src/dag-node-utils.js

+  if (node.data && node.data.length > 0) {
+    pbn.Data = node.data
+  } else {
+    pbn.Data = null // new Buffer(0)


please don't change this, it needs to be a Buffer of size 0, we had a problem in the past because the serialized versions were different when data was empty (go vs js)

I didn't change this, it was like that in the original PR. Will change it to Buffer(0)

Actually, not gonna change this back. If I do, one of the tests starts failing:

1) Node.js Tests DAGNode create an empty node: AssertionError: expected 2 to equal 0 + expected - actual -2 +0 at DAGNode.create (test/dag-node-test.js:93:33)

Which way should it be? Change the code AND test, or leave it as it is?

daviddias · 2016-11-16T10:59:28Z

src/dag-node-utils.js

+exports.addLink = addLink
+exports.removeLink = removeLink
+exports.toDAGLink = toDAGLink
+exports.util = {


because of IPLD Format definition looking for the util

daviddias · 2016-11-16T11:03:51Z

src/dag-node-utils.js

+const DAGNode = require('./dag-node')
+const DAGLink = require('./dag-link')
+
+const hash = (type, data, cb) => multihashing(data, type, cb)


If this is the case, just switch the arguments where hash is called and make it be multihashing

daviddias · 2016-11-16T11:43:39Z

README.md

@@ -50,9 +49,11 @@ dagPB.util
 Create a new DAGNode

 ```JavaScript
-var node = new dagPB.DAGNode([<data>, <[links]>])
+dagPB.DAGNode.create([<data>, <[links]>, hashAlgorithm, callback]) 


still needs to be updated

daviddias · 2016-11-16T11:43:51Z

README.md

 ```

+#### TODO: update the functions docs from here on out
+


still needs to be updated

daviddias · 2016-11-16T11:45:05Z

src/dag-link.js

+  }
+
+  set multihash (multihash) {
+    throw new Error(`Can't set property: 'multihash' is immutable`)


daviddias · 2016-11-16T11:48:12Z

src/dag-node.js

-      }
+    this._data = data || new Buffer(0)
+    this._links = links || []
+    this._serialized = serialized || new Buffer(0) // TODO: default serialized object


needs to be done

daviddias · 2016-11-16T11:48:49Z

src/dag-node.js

+    this._data = data || new Buffer(0)
+    this._links = links || []
+    this._serialized = serialized || new Buffer(0) // TODO: default serialized object
+    this._multihash = multihash || new Buffer(0) // TODO: default multihash object


what is the default multihash? I don't believe this is correct

daviddias · 2016-11-16T12:00:48Z

src/utils.js

+      // It's a multihash
+      newLink = new DAGLink(null, dagNode.size, nodeOrMultihash)
+    }
+  }


This piece is incorrect, (also already gave reviews to this before). Again see line 67 to 97 #6 (comment)

Don't delete code + comments just because, information gets lost

Also, the addLink function, will need the size of link if just a name and multihash are passed.

Please provide details as to what is incorrect when reviewing. Just saying this is incorrect is not helpful feedback. My original comment was "I don't understand what is meant here with your comments, please review if this is correct". Provide possible solutions, not just "this is not ok".

You are correct, apologies for not being clear.

Right now, the thing that is missing from what I see is:

We need to also pass the size of the link as an argument, because when we pass a name+multihash, we also need to pass the size of that link, otherwise the size of this node will be miscalculated

This method can only be considered complete if the tests for adding and removing links are passing, right now they are commented out and with an incomplete migration, which is why I had this part in a TODO too. So, we need the tests to be migrated to this new API and making sure it is all correct

daviddias · 2016-11-16T12:01:32Z

src/utils.js

+
+  if (newLink) {
+    links.push(newLink)
+    sort(links, linkSort)


create does the sorting, we don't need to sort it twice

daviddias · 2016-11-16T12:02:00Z

src/utils.js

+
+function removeLink (dagNode, nameOrMultihash, callback) {
+  const data = _cloneData(dagNode)
+  let links = _cloneLinks(dagNode)


it can be const

It can't, see the code couple of lines below.

Doing push on array doesn't require it to be let

daviddias · 2016-11-16T12:02:28Z

src/utils.js

+    return new DAGLink(link.Name, link.Tsize, link.Hash)
+  })
+
+  sort(links, linkSort)


create does sorting

daviddias · 2016-11-16T12:05:46Z

src/utils.js

+  if (node.data && node.data.length > 0) {
+    pbn.Data = node.data
+  } else {
+    pbn.Data = null//new Buffer(0)


(also, already had reviewed) this is a bug that was already caught in the past, null is not the same as new Buffer(0) for protobuf serialization

Yes and my question was: "Which way should it be?". Should I change the test so that it passed when this is changed to new Buffer(0)?

It should be as it was, new Buffer(0), it is about how protobuf serialization libraries work, we know for a fact that our 'empty objects' are different from go ones if we don't have a empty buffer when we go and serialize

daviddias · 2016-11-16T12:14:37Z

One more review. Thank you for enduring throughout this PR, it is really good to get you reviewing all of this as a consumer of all the API. Seems also that you are getting a speed up in IPLD from it too :)

Another comment: @haadcode I've reviewed at least 3 times the same 4 or 5 parts of this PR, some of which were bugs and still are there. It would save a lot of time if you make changes in smaller commits, avoid renaming files (util.js -> utils.js) and review every piece of feedback before asking for more, I feel a lot of information is getting lost and a lot of energy spent as well.

haadcode · 2016-11-16T12:21:15Z

Another comment: @haadcode I've reviewed at least 3 times the same 4 or 5 parts of this PR, some of which were bugs and still are there. It would save a lot of time if you make changes in smaller commits, avoid renaming files (util.js -> utils.js) and review every piece of feedback before asking for more, I feel a lot of information is getting lost and a lot of energy spent as well.

Perhaps something went wrong in Github, but the "same 3 times" was with questions from me to you to clarify your comments. I have NOT committed anything without reviewing your feedback.

haadcode · 2016-11-16T13:32:01Z

Ok, one more round. Addressed everything in the feedback except promises. Everything else is "final" as far as I'm concerned.

haadcode · 2016-11-16T13:54:06Z

After the CR and the required changes, I think the code is not as clean anymore as the original code in the PR was.

Furthermore, the IPLD format api with the .resolver and .util (especially the util) doesn't make sense and should be reconsidered. Things are a) more coupled and b) don't provide clean namespacing/naming and structure and even makes the module internal implementation to adhere to those decisions.

However, for the sake of urgency, I'll leave this as is. There's been way too much bikeshedding regarding code style as oppose to functionality, correctness and API design and that takes away from motivation and from getting stuff done in timely manner. We need to stop using the argument "because it's like that elsewhere" as a catch-all when it clearly produces lower quality code and design.

Take what you want from this PR.

daviddias · 2016-11-16T16:32:00Z

However, for the sake of urgency, I'll leave this as is. There's been way too much bikeshedding regarding code style as oppose to functionality

Sounds good 👍

Ok, one more round. Addressed everything in the feedback except promises. Everything else is "final" as far as I'm concerned.

This sound like functionality to me, what made you drop it?

#7 (comment)

This change will cause errors.

#7 (comment)

❤️

Other minor things:

Refactor feat/immutable PR #7 (comment)
lint is failing https://travis-ci.org/ipld/js-ipld-dag-pb/jobs/176372009#L1282, for tests we are ok with having many levels (till 8), so we do this https://github.com/ipfs/interface-ipfs-core/blob/master/src/files.js#L2

…lder, make tests pass again

… write more tests

daviddias · 2016-11-20T16:38:07Z

As discussed, I finished the last points of review in this PR, fixed tests, updated docs and also give a spin to a better addLink API

daviddias · 2016-11-24T14:47:18Z

src/util.js

  const pbn = proto.PBNode.decode(data)

  const links = pbn.Links.map((link) => {
    return new DAGLink(link.Name, link.Tsize, link.Hash)
  })

-  stable.inplace(links, exports.linkSort)


daviddias reviewed Nov 14, 2016

View reviewed changes

src/dag-link.js

return this._multihash ? mh.toB58String(this._multihash) : undefined

}

set hash() {

Copy link

Member

daviddias Nov 14, 2016

Choose a reason for hiding this comment

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

s/hash/multihash

daviddias reviewed Nov 14, 2016

View reviewed changes

src/dag-link.js

throw ImmutableError

}

get hash() {

Copy link

Member

daviddias Nov 14, 2016

Choose a reason for hiding this comment

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

s/hash/multihash

daviddias reviewed Nov 14, 2016

View reviewed changes

daviddias suggested changes Nov 14, 2016

View reviewed changes

haadcode changed the title ~~Rebase on feat/immutable~~ Refactor feat/immutable PR Nov 14, 2016

daviddias suggested changes Nov 16, 2016

View reviewed changes

dignifiedquire reviewed Nov 16, 2016

View reviewed changes

daviddias reviewed Nov 16, 2016

View reviewed changes

daviddias suggested changes Nov 16, 2016

View reviewed changes

daviddias reviewed Nov 16, 2016

View reviewed changes

daviddias suggested changes Nov 16, 2016

View reviewed changes

haadcode and others added 2 commits November 20, 2016 13:44

feat(refactor): new API proposal

b56d797

feat: refactor, structure code, make DAGNode funcs inside the same fo…

ea904a7

…lder, make tests pass again

daviddias force-pushed the feat/immutable-haad branch from 3c235fe to ea904a7 Compare November 20, 2016 14:30

daviddias added 5 commits November 20, 2016 14:35

feat: add a same create pattern api for DAGLink

6a3531d

fix: that linting

301d0b0

docs: revamp README documentation to the new API

6e3ab23

fix: DAGLink tests

eec00da

feat+fix: complete addLink and rmLink functions + fix their tests and…

710e53f

… write more tests

daviddias merged commit 8f7aadc into feat/immutable Nov 20, 2016

daviddias deleted the feat/immutable-haad branch November 20, 2016 16:38

daviddias reviewed Nov 24, 2016

View reviewed changes

		@@ -2,28 +2,57 @@

		const mh = require('multihashes')

		const ImmutableError = new Error('Immutable property')

Refactor feat/immutable PR #7

Refactor feat/immutable PR #7

Conversation

haadcode commented Nov 14, 2016

haadcode commented Nov 14, 2016

Choose a reason for hiding this comment

haadcode commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Nov 14, 2016

daviddias commented Nov 14, 2016 • edited Loading

daviddias left a comment

Choose a reason for hiding this comment

haadcode commented Nov 14, 2016

haadcode commented Nov 14, 2016

haadcode commented Nov 14, 2016

daviddias commented Nov 14, 2016

haadcode commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Nov 14, 2016 •

edited

Loading