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

doc: format try...catch consistently #28481

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 30, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 30, 2019
@@ -311,7 +311,7 @@ it is compiled (i.e. sometimes it may be in `./build/Debug/`), Addons can use
the [bindings][] package to load the compiled module.

While the `bindings` package implementation is more sophisticated in how it
locates Addon modules, it is essentially using a try-catch pattern similar to:
locates Addon modules, it is essentially using a `trycatch` pattern similar to:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: should we use 3 dots instead of ellipsis character?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No personal preference - I just copied what was already used in the errors docs.

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 also fine either way, just pointed out because 3 dots are used on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've replaced the ellipsis throughout the docs and tests. I didn't update the CHANGELOGS though.

Copy link
Member

Choose a reason for hiding this comment

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

Personal opinion: 3 dots are good for code, but ellipsis really do look nicer in prose text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we use 3 dots instead of ellipsis character?

Personal opinion: 3 dots are good for code, but ellipsis really do look nicer in prose text.

I don't want to get hung up on this. I think I'll just drop the second commit from this PR.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 2, 2019

Leaving the ellipsis vs. three dots debate for another time. Green Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3735/

PR-URL: nodejs#28481
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@cjihrig cjihrig merged commit cce78bb into nodejs:master Jul 2, 2019
@cjihrig cjihrig deleted the try-catch branch July 2, 2019 18:08
targos pushed a commit that referenced this pull request Jul 20, 2019
PR-URL: #28481
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants