doc: remove outdated try/catch statements and use const #3087
Conversation
fix description about crypto.randomBytes
| low on entropy. | ||
| normally never take longer than a few milliseconds. | ||
| Under normal circumstances, the only error thrown from this is from RAND_bytes(), which throws when it doesn't have enough entropy. | ||
| However, with CheckEntropy, this will block until the system has enough entropy for the OpenSSL pool. |
There was a problem hiding this comment.
I don't think that's very enlightening for people not familiar with openssl or the code base. I don't really have suggestions on how to reword it, just that it's not very helpful now (and the part about CheckEntropy() is arguably wrong in a nuanced way.)
Also, please wrap lines at 80 columns.
There was a problem hiding this comment.
Perhaps, @JungMinu, you could update the code block instead? The note is mostly correct about the current behavior. Maybe remove the try / catch block in the example?
|
The commit log could be be a little more descriptive. Ideally, you shouldn't have to look at the diff to know what a commit changed. |
|
@bnoordhuis Thanks, I will update |
brendanashworth
added
crypto
|
@brendanashworth Thanks for your comment, I would love to 😄 |
|
@JungMinu assuming you've setup locally with git, you can always add new changes as new commits and push them to github. It will automatically update this PR, which is easier for everyone. |
|
@brendanashworth Yes, I will. |
remove outdated examples about crypto.randomBytes to make it clear
|
@brendanashworth Based on your comments, I removed |
|
@brendanashworth @bnoordhuis Please review |
|
I speculate that @brendanashworth's suggestion was to remove the try/catch statement, not the call to |
fix outdated examples about crypto.randomBytes to make it clear
|
@bnoordhuis Thanks, I've updated |
JungMinu
changed the title
JungMinu
changed the title
JungMinu
changed the title
The code shows that it throws on a lack of entropy, but the note below says that it does not. Remove outdated `try/catch`statements in sync examples about crypto.randomBytes to make it clear
|
@bnoordhuis I've updated commit log and description, Thanks 😄 |
The code shows that it throws on a lack of entropy, but the note below says that it does not. Remove outdated try/catchstatements in sync examples about crypto.randomBytes to make it clear
The code shows that it throws on a lack of entropy, but the note below says that it does not. Remove outdated `try/catch`statements in sync examples about crypto.randomBytes to make it clear
| // handle error | ||
| // most likely, entropy sources are drained | ||
| } | ||
| // Implementation of handling errors is recommended. |
There was a problem hiding this comment.
Does this mean there should be a try/catch? randomBytes() will never throw anymore, right?
The code shows that it throws on a lack of entropy, but the note below says that it does not. Remove outdated `try/catch`statements in sync examples about crypto.randomBytes to make it clear
|
@Fishrock123 Sorry, I made a mistake. |
|
@Fishrock123 Thanks for your comment 😄 |
|
LGTM |
|
Perfect, also LGTM! |
| // handle error | ||
| // most likely, entropy sources are drained | ||
| } | ||
| var buf = crypto.randomBytes(256); |
There was a problem hiding this comment.
May I suggest to use const?
|
LGTM with take-it or leave-it suggestion |
The code shows that it throws on a lack of entropy, but the note below says that it does not. Remove outdated try/catchstatements in sync examples about crypto.randomBytes to make it clear. In addition, use `const` instead of `var` for constant variable.
|
@thefourtheye Thanks, I've updated |
JungMinu
changed the title
The code shows that it throws on a lack of entropy, but the note below says that it does not.
Remove outdated
try/catchstatements in sync examples about crypto.randomBytes to make it clear