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

resolve #261, explain args to callback to #del #293

Merged
merged 2 commits into from
Nov 15, 2014

Conversation

bewest
Copy link
Contributor

@bewest bewest commented Nov 15, 2014

Per bug report, example crafted from test.

Howdy, thanks for your work. I was reviewing a bunch of code, and noticed this note and thought it was easy enough.

Per bug report, example crafted from test.
@rvagg
Copy link
Member

rvagg commented Nov 15, 2014

example has a few problems:

  • callback function is named, this is not consistent with the rest of the examples (naming functions is good practice but we should be consistent, either name everything or nothing)
  • callback content doesn't account for the non error case, it just assumes there will always be an error
  • example needs to be somewhat real-world-ish, meaning that it shouldn't console.* and also throw is rarely going to be the way people deal with errors, it's much better to just have something like:
if (err)
  // deal with error

Thanks for the feedback, @rvagg.
Based on your feedback and some double checking via
https://github.com/rvagg/node-leveldown/blob/5d552d750170bedd0656d2e6d82673556fa0ea0d/deps/leveldb/leveldb-1.17.0/db/db_impl.cc#L1437-L1441

I've gone with a comment similar to the preceding stanza.
Also removed the curly braces.
@rvagg
Copy link
Member

rvagg commented Nov 15, 2014

Nice, I'm going to assume that since this is just a simple doc fix that nobody else is going to object so I'll merge. Thanks!

rvagg added a commit that referenced this pull request Nov 15, 2014
resolve #261, explain args to callback to #del
@rvagg rvagg merged commit 62d2c05 into Level:master Nov 15, 2014
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