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

Add functionality to copy feed and append to it. #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

benjaminettori
Copy link

No description provided.

@benjaminettori
Copy link
Author

@flyingzumwalt It seems node did not start supporting promises natively until v 0.11, which means the build for v 0.10 fails. Is this an issue?

Change module export name.
Squash commits for appending feed
@flyingzumwalt
Copy link
Collaborator

@benjaminettori I think it's fine for us to only support node 0.11 and higher. Remove 0.10 from the travis.yml and update the Readme to call out the dependency on node >= 0.11.

}
})
})
}

Choose a reason for hiding this comment

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

this might append blocks in the wrong order as this is async. you probably wanna do this

loop(null)
function loop (err) {
  if (err) return reject(err)
  refFeed.get(newFeed.blocks, function (err, block) {
    if (err) return cb(err)
    if (!block) return resolve() // done
    newFeed.append(block, loop)
  })
}

@flyingzumwalt
Copy link
Collaborator

@mafintosh you commented in irc that you're not a fan of promises for server-side code. Could you elaborate? I think @benjaminettori chose to use promises because it was a clearer way to handle nested callbacks. @benjaminettori could you provide an example of a place where the code would get confusing without promises?

@benjaminettori
Copy link
Author

@flyingzumwalt Actually I may have a way to do this without promises, I will submit another PR if I finalize it. I was using promises because I wanted to return a feed object, but this is probably not the best way to approach the issue. Thanks @mafintosh for catching that mistake and for the clever code snippet.

@benjaminettori
Copy link
Author

@flyingzumwalt I refactored the code and added methods to replicate and append to feeds without using promises. I also added some tests around these new methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants