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: modernize and fix code examples in modules.md #12224

Closed
wants to merge 8 commits into from
Closed

doc: modernize and fix code examples in modules.md #12224

wants to merge 8 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 5, 2017

Checklist
Affected core subsystem(s)

doc, modules

  • Replace var by const.
  • Fix semicolons.
  • Add missing code marks.
  • Unify quotes.
  • Comment out ellipsis.
  • Use object destructuring.
  • Use exponentiation operator.
  • Replace snake_case by camelCase.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Apr 5, 2017
@@ -22,9 +22,9 @@ Here are the contents of `circle.js`:
```js
const { PI } = Math;

exports.area = (r) => PI * r * r;
exports.area = r => PI * r * r;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: not sure about other docs examples but this is inconsistent with core style where () is also used for a single argument.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 7, 2017

Choose a reason for hiding this comment

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

@lpinca There is a nuance here. I mean somethin like this advice: when we have implicite return and the function body is not in braces, omit parentheses around one argument. I've searched the https://nodejs.org/api/all.html and have found only 2 such cases, and they are better be rewritten with braces, because they do not use returned value and have side effects:

https://github.com/nodejs/node/blame/47f8f7462fb198aa27ede602c43786bdbfda37a2/doc/api/process.md#L313

https://github.com/nodejs/node/blame/82ef00cc0a36492808badb23d4d79ed303412943/doc/api/stream.md#L1757

Then I've searched the core libs and have found only one fragment with omitted parentheses and dozens with them. So I shall revert this cange to conform with core.

area: () => width * width
};
};
module.exports = width => ({
Copy link
Member

Choose a reason for hiding this comment

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

Nit: no strong opinions here but I think the original is easier to grok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpinca I shall revert this too.

((module, exports) => {
// Your module code here. In this example, define a function.
function some_func() {};
function some_func() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good opportunity to convert this identifier to camelCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aqrln Sure)

Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

LGTM. Would be also nice to backport it to LTS branches taking into account which features are supported by those Node versions (e.g., exponentiation operator change would need to be reverted).

@vsemozhetbyt
Copy link
Contributor Author

Landed in 166a156

@vsemozhetbyt vsemozhetbyt deleted the modules.md branch April 8, 2017 13:06
vsemozhetbyt added a commit that referenced this pull request Apr 8, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Use exponentiation operator.
* Replace snake_case by camelCase.

PR-URL: #12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Use exponentiation operator.
* Replace snake_case by camelCase.

PR-URL: nodejs#12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

@vsemozhetbyt would you be willing to make a backport PR to v6.x?

@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins I shall try.

@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins Done.

If I get it right, I had only to remove the 'use exponentiation operator' part.

MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Replace snake_case by camelCase.

Backport-PR-URL: #12500
PR-URL: #12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Replace snake_case by camelCase.

Backport-PR-URL: #12500
PR-URL: #12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Replace snake_case by camelCase.

Backport-PR-URL: #12500
PR-URL: #12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 29, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
* Replace `var` by `const`.
* Fix semicolons.
* Add missing code marks.
* Unify quotes.
* Comment out ellipsis.
* Use object destructuring.
* Replace snake_case by camelCase.

Backport-PR-URL: nodejs/node#12500
PR-URL: nodejs/node#12224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
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. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants