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

Awesome IPLD Endeavour #89

Merged
merged 2 commits into from
Oct 26, 2016
Merged

Awesome IPLD Endeavour #89

merged 2 commits into from
Oct 26, 2016

Conversation

daviddias
Copy link
Member

  • remove extension support (no need with CID)
  • blockstore starts getting blockBlobs instead of blocks (so that it can know the right key)

@dignifiedquire
Copy link
Member

Why not add cid support directly to Block, The current {block, cid} constructs seems akward and easy to break accidentally.

@daviddias daviddias mentioned this pull request Oct 3, 2016
36 tasks
@daviddias
Copy link
Member Author

daviddias commented Oct 3, 2016

I scratched my head a lot thinking about this :). Although having a .cid() on the block would be super sweet, you can't really tell which hash function to use, otherwise, it breaks the Block abstraction.

Only the Block Service, hinted by the caller with the CID, can tell which data type and hash function should it use for the data it is looking for.

@@ -2,7 +2,7 @@
"name": "ipfs-repo",
"version": "0.9.1",
"description": "IPFS Repo implementation",
"main": "lib/index.js",
"main": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

TODO: change before merge

@@ -5,16 +5,16 @@ const pull = require('pull-stream')
const Lock = require('lock')
const base32 = require('base32.js')
const path = require('path')
const write = require('pull-write')
const pullWrite = require('pull-write')
Copy link
Member

Choose a reason for hiding this comment

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

why?

function multihashToPath (multihash, extension) {
extension = extension || 'data'
function multihashToPath (multihash) {
const extension = 'data'
Copy link
Member

Choose a reason for hiding this comment

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

if this is fixed, why not make it a constant like PREFIX_LENGTH

const tasks = blockBlobs.map((blockBlob) => {
return (cb) => {
writeBlock(blockBlob, (err, meta) => {
if (err) {
Copy link
Member

Choose a reason for hiding this comment

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

the nesting is strong in this one, maybe simplify this somehow?


cb()
})
}
Copy link
Member

Choose a reason for hiding this comment

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

👎 this change makes it harder to read, and is a matter of personal preference

Copy link
Member Author

Choose a reason for hiding this comment

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

I can agree on that for line 44, however, for line 44, the left (previous) is way harder to read because the way the 3 functions are nested is not entirely clear.

})

// TODO ??Why does a putStream need to be a source as well??
Copy link
Member Author

Choose a reason for hiding this comment

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

@dignifiedquire could you clarify on this one?

})

// TODO ?? Why does a putStream need to be a source as well??
Copy link
Member Author

Choose a reason for hiding this comment

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

… that now it receives the key in which it should store it
@daviddias
Copy link
Member Author

for record, @dignifiedquire approved

@daviddias daviddias merged commit 32e8fca into master Oct 26, 2016
@daviddias daviddias deleted the awesome-ipld branch October 26, 2016 10:14
@daviddias daviddias removed the status/in-progress In progress label Oct 26, 2016
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.

2 participants