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

Added setMany() functionality #74

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

Conversation

Rob--
Copy link

@Rob-- Rob-- commented Jul 17, 2017

The PR included both the core method, the tests and an updated README.

@Rob-- Rob-- mentioned this pull request Jul 17, 2017
Copy link
Member

@jviotti jviotti left a comment

Choose a reason for hiding this comment

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

I left a few comments. The PR overall looks very, very nice, so just some thoughts on how to improve it and reduce some boilerplate!

}

if (entries.length > 0) {
process();
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove a lot of boilerplate by reusing async's parallel function (http://caolan.github.io/async/docs.html#parallel), or even parallelLimit, so we only have X amount of writes at any given time.

Copy link
Author

@Rob-- Rob-- Jul 18, 2017

Choose a reason for hiding this comment

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

I replied on #73 about using async, but the if statement is not necessary at all, it is just to ensure that in the event someone does storage.setMany({}) that an error will not be thrown. I've never used async.parallel so I'm unsure if would be able to handle an event in which no keys are passed to the function.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if would be able to handle an event in which no keys are passed to the function.

Fair enough. The current approach looks good then :)


exports.set(key, value, function(error) {
if (error) {
callback({ error: error, key: key }, entries.length === 0);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be good if instead of simply returning if things were successful or not, we would return the list of properties that failed (or the ones that succeeded), so in the case of an error, the user knows which ones to retry?

Copy link
Author

Choose a reason for hiding this comment

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

That was originally what I had planned. With the way I ended up doing it, the user will still know which ones to retry (it will set the keys one by one, and every time there is an error it will invoke the callback which is why the callback takes a second parameter finished to let the user know when all the keys have been set).

It's up to you. If we did it your way, would we simply return a list of failed keys (['foo', 'bar']) or the errors that they threw too ({ foo: error1, bar: error2 })?

Copy link
Member

Choose a reason for hiding this comment

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

I see. I think we should return a list of failed keys along with their respective errors, and only call the final callback once.

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