FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: remove outdated try/catch statements and use const by JungMinu · Pull Request #3087 · nodejs/node · GitHub

/ node Public

doc: remove outdated try/catch statements and use const - #3087

Closed
JungMinu wants to merge 8 commits into
nodejs:masterfrom
JungMinu:patch-17
ConversationCommits (8)ChecksFiles changed
Closed

doc: remove outdated try/catch statements and use const#3087
JungMinu wants to merge 8 commits into
nodejs:masterfrom
JungMinu:patch-17

Conversation

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member

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

doc: fix crypto.randomBytes
54cd451 
fix description about crypto.randomBytes
JungMinu mentioned this pull request Sep 27, 2015
Closed
bnoordhuis reviewed Sep 27, 2015
View reviewed changes
Comment thread doc/api/crypto.markdown Outdated

bnoordhuis Sep 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

brendanashworth Sep 27, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

bnoordhuis commented Sep 27, 2015

Copy link
Copy Markdown
Member

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.

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis Thanks, I will update

brendanashworth added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Sep 27, 2015

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@brendanashworth Thanks for your comment, I would love to 😄

brendanashworth commented Sep 27, 2015

Copy link
Copy Markdown
Contributor

@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.

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@brendanashworth Yes, I will.

doc: update examples about crypto.randomBytes
0d0154d 
remove outdated examples about crypto.randomBytes to make it clear

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@brendanashworth Based on your comments, I removed try / catch block.

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@brendanashworth @bnoordhuis Please review

bnoordhuis commented Sep 27, 2015

Copy link
Copy Markdown
Member

I speculate that @brendanashworth's suggestion was to remove the try/catch statement, not the call to crypto.randomBytes().

doc: update examples about crypto.randomBytes
a8af4f3 
fix outdated examples about crypto.randomBytes to make it clear

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis Thanks, I've updated

JungMinu changed the title doc: fix crypto.randomBytes doc: Remove outdated try/catchstatements in sync examples Sep 27, 2015
JungMinu changed the title doc: Remove outdated try/catchstatements in sync examples doc: Remove outdated try/catch statements in sync examples Sep 27, 2015
JungMinu changed the title doc: Remove outdated try/catch statements in sync examples doc: Remove outdated try/catch statements in sync Sep 27, 2015
doc: remove outdated try/catchstatements in sync
69b5fb9 
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

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis I've updated commit log and description, Thanks 😄

doc: remove outdated 'try/catch' statement in sync
77047dd 
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
doc: remove outdated 'try/catch' statement in sync
c723c46 
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 reviewed Sep 27, 2015
View reviewed changes
Comment thread doc/api/crypto.markdown Outdated

Fishrock123 Sep 27, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does this mean there should be a try/catch? randomBytes() will never throw anymore, right?

doc: remove outdated 'try/catch' statement in sync
879fa25 
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

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 Sorry, I made a mistake.
I've updated.

JungMinu commented Sep 27, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 Thanks for your comment 😄

bnoordhuis commented Sep 28, 2015

Copy link
Copy Markdown
Member

LGTM

brendanashworth commented Sep 28, 2015

Copy link
Copy Markdown
Contributor

Perfect, also LGTM!

thefourtheye reviewed Sep 29, 2015
View reviewed changes
Comment thread doc/api/crypto.markdown Outdated

thefourtheye Sep 29, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

May I suggest to use const?

thefourtheye commented Sep 29, 2015

Copy link
Copy Markdown
Contributor

LGTM with take-it or leave-it suggestion

doc: fix outdated 'try/catch' statement in sync
25cf161 
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.

JungMinu commented Sep 29, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye Thanks, I've updated

JungMinu changed the title doc: Remove outdated try/catch statements in sync doc: remove outdated try/catch statements and use const Sep 29, 2015
brendanashworth pushed a commit that referenced this pull request Oct 1, 2015
doc: fix outdated 'try/catch' statement in sync
d32363f 
Fixes description about crypto.randomBytes.

Fixes: #3081
PR-URL: #3087
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

brendanashworth commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Landed in d32363f, thanks @JungMinu!

rvagg pushed a commit that referenced this pull request Oct 2, 2015
doc: fix outdated 'try/catch' statement in sync
f92aee7 
Fixes description about crypto.randomBytes.

Fixes: #3081
PR-URL: #3087
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg mentioned this pull request Oct 3, 2015
Closed
aliscco mentioned this pull request Jun 22, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL