GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — doc: modernize and fix code examples in modules.md by vsemozhetbyt · Pull Request #12224 · nodejs/node

Conversation

vsemozhetbyt commented Apr 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Checklist

documentation is changed or added
commit message follows commit guidelines

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vsemozhetbyt added 6 commits

April 5, 2017 04:16


          doc: replace var by const in modules.md

f00cef5


          doc: fix semicolons in modules.md

61c10a3


          doc: add missing code marks in modules.md

060b787


          doc: unify quotes in modules.md

9dd7183


          doc: comment out ellipsis in modules.md

7848aef


          doc: use object destructuring in modules.md

e1f8997

nodejs-github-bot added doc

Issues and PRs related to Node.js documentation.

module Issues and PRs related to the module subsystem. labels

Apr 5, 2017

lpinca approved these changes

Apr 7, 2017

View reviewed changes

Comment thread

doc/api/modules.md Outdated

lpinca Apr 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt reacted with thumbs up emoji

All reactions

👍 1 reaction

vsemozhetbyt Apr 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

doc/api/modules.md Outdated

lpinca Apr 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt reacted with thumbs up emoji

All reactions

👍 1 reaction

vsemozhetbyt Apr 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          doc: use exponentiation operator in modules.md

659a802

aqrln reviewed

Apr 7, 2017

View reviewed changes

Comment thread

doc/api/modules.md Outdated

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

aqrln Apr 7, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt reacted with thumbs up emoji

All reactions

👍 1 reaction

vsemozhetbyt Apr 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@aqrln Sure)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          doc: replace snake_case by camelCase in modules.md

caf094f

aqrln approved these changes

Apr 7, 2017

View reviewed changes

aqrln left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vsemozhetbyt added lts-watch-v4.x labels

Apr 7, 2017

vsemozhetbyt commented Apr 8, 2017

Copy link

Copy Markdown

Contributor Author

Landed in 166a156

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt closed this

Apr 8, 2017

vsemozhetbyt deleted the modules.md branch

April 8, 2017 13:06

vsemozhetbyt added a commit that referenced this pull request

Apr 8, 2017


          doc: modernize and fix code examples in modules.md

166a156

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

italoacasas pushed a commit to italoacasas/node that referenced this pull request

Apr 10, 2017


          doc: modernize and fix code examples in modules.md

3e8991c

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

italoacasas mentioned this pull request

Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks done

MylesBorins added backport-requested-v6.x and removed lts-watch-v4.x labels

Apr 18, 2017

MylesBorins commented Apr 18, 2017

Copy link

Copy Markdown

Contributor

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

vsemozhetbyt reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt commented Apr 18, 2017

Copy link

Copy Markdown

Contributor Author

@MylesBorins I shall try.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt mentioned this pull request

Apr 19, 2017

(v6.x backport) doc: modernize and fix code examples in modules.md #12500

Closed

2 tasks done

vsemozhetbyt commented Apr 19, 2017

Copy link

Copy Markdown

Contributor Author

@MylesBorins Done.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins pushed a commit that referenced this pull request

Apr 24, 2017


          doc: modernize and fix code examples in modules.md

7d029ae

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels

Apr 24, 2017

MylesBorins pushed a commit that referenced this pull request

Apr 24, 2017


          doc: modernize and fix code examples in modules.md

d15188f

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Apr 24, 2017


          doc: modernize and fix code examples in modules.md

276267f

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

MylesBorins mentioned this pull request

Apr 29, 2017

v6.10.3 proposal #12498

Merged

andrew749 pushed a commit to michielbaird/node that referenced this pull request

Jul 19, 2017


          doc: modernize and fix code examples in modules.md

54998e9

* 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 <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>

Reviewers

lpinca

lpinca approved these changes

+1 more reviewer

aqrln

aqrln approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc

Issues and PRs related to Node.js documentation.

module

Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

vsemozhetbyt commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

lpinca Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Apr 8, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

vsemozhetbyt commented Apr 18, 2017

Uh oh!

vsemozhetbyt commented Apr 19, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vsemozhetbyt commented Apr 5, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

vsemozhetbyt Apr 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.