Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

feat+fix: implement .tree and fix how the resolver was working #29

Merged
merged 3 commits into from
Mar 13, 2017

Conversation

daviddias
Copy link
Member

In the process of implementing .tree, I've realized how we fell into the trap of 'transforming' the original data for convenience, for example, it was not possible to access the fields 'Name' or 'Tsize' of the protobuf, because we were handling the links as a direct jump to other objects.

This PR:

  • Fixes the resolver
  • Adds more tests
  • Implements tree
  • Implements isLink

@daviddias daviddias added the status/in-progress In progress label Mar 11, 2017
@daviddias
Copy link
Member Author

also make sure to check ipld/interface-ipld-format#5

@nicola
Copy link
Member

nicola commented Mar 13, 2017

sgtm

}
// TODO by enabling something to resolve through link name, we are
// applying a transformation (a view) to the data, confirm if this
// is exactly what we want
Copy link
Member

Choose a reason for hiding this comment

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

is this a todo or a question? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Ask the question:.... :D

values[i] = {
hash: link.multihash,
name: link.name,
size: link.size
Copy link
Member

Choose a reason for hiding this comment

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

what is the size needed for?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the size of this branch of the graph so far. It is needed to avoid traversing graphs over and over. go-ipfs has it since forever.

src/resolver.js Outdated
@@ -1,10 +1,9 @@
'use strict'

const util = require('./util')
const bs58 = require('bs58')
// const bs58 = require('bs58')
Copy link
Member

Choose a reason for hiding this comment

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

please delete if you are not using it

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

src/resolver.js Outdated
split.shift()
split.shift()

remainderPath = split.join('/')
Copy link
Member

Choose a reason for hiding this comment

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

these 4 lines could be simply

remainderPath = split.slice(3).join('/')

if (node.data && node.data.length > 0) {
paths.push({ path: 'data', value: node.data })
}
paths.push('Data')
Copy link
Member

Choose a reason for hiding this comment

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

The above can be simplified to

const links = node.links.map((l, i) => ([
  `Links/${i}/Name`,
  `Links/${i}/Tsize`,
  `Links/${i}/Hash`
])
links.push('Data')
const paths = Array.prototype.concat.apply(['Links'], links)

src/resolver.js Outdated

callback(null, paths)
})
}

/*
* isLink: returns the Link if a given path in a block is a LInk, false otherwise
Copy link
Member

Choose a reason for hiding this comment

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

typo LInk

src/resolver.js Outdated
}

if (typeof result.value === 'object' && result.value['/']) {
// result.value['/'] = bs58.encode(result.value['/'])
Copy link
Member

Choose a reason for hiding this comment

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

please remove if not used

@daviddias daviddias merged commit 1b7c3e0 into master Mar 13, 2017
@daviddias daviddias deleted the feat/tree branch March 13, 2017 11:53
@daviddias daviddias removed the status/in-progress In progress label Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants