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

test,doc: do not indicate that non-functions "return" values by Trott · Pull Request #17267 · nodejs/node · GitHub

/ node Public

test,doc: do not indicate that non-functions "return" values - #17267

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:return-no-return
ConversationCommits (1)ChecksFiles changed
Closed

test,doc: do not indicate that non-functions "return" values#17267
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:return-no-return

Conversation

Trott commented Nov 23, 2017

Copy link
Copy Markdown
Member

Update documentation for test common module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

Checklist
Affected core subsystem(s)

test doc

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 23, 2017
vsemozhetbyt approved these changes Nov 23, 2017
View reviewed changes
Comment thread test/common/README.md Outdated

vsemozhetbyt Nov 23, 2017

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

A space after the *

Comment thread test/common/README.md Outdated

vsemozhetbyt Nov 23, 2017

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

false -> `false`?

Comment thread test/common/README.md Outdated

vsemozhetbyt Nov 23, 2017

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

true -> `true`?

Comment thread test/common/README.md Outdated

vsemozhetbyt Nov 23, 2017

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

are supported?

Comment thread test/common/README.md Outdated

vsemozhetbyt Nov 23, 2017

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

An optional nit: unify articles (a/the/-) for such descriptions?

vsemozhetbyt added the doc Issues and PRs related to the documentations. label Nov 23, 2017
lpinca approved these changes Nov 23, 2017
View reviewed changes

lpinca left a comment

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

LGTM with @vsemozhetbyt's comments addressed.

Trott commented Nov 29, 2017

Copy link
Copy Markdown
Member Author

Rebased to resolve merge conflict, and all nits (except for the one labeled "optional") addressed.

Trott commented Nov 29, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11789/

gibfahn reviewed Nov 29, 2017
View reviewed changes
Comment thread test/common/README.md Outdated

gibfahn Nov 29, 2017

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

Aren't we losing information here (type of the property)?

Trott Dec 1, 2017

Copy link
Copy Markdown
Member Author

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'll follow the convention in our docs and restore these lines but remove the word return.

test,doc: do not indicate that non-functions "return" values
cd640af 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

Trott commented Dec 1, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11837/

Trott added a commit to Trott/io.js that referenced this pull request Dec 1, 2017
test,doc: do not indicate that non-functions "return" values
dd629ef 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: nodejs#17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Trott commented Dec 1, 2017

Copy link
Copy Markdown
Member Author

Landed in dd629ef

Trott closed this Dec 1, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test,doc: do not indicate that non-functions "return" values
f94dc0a 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test,doc: do not indicate that non-functions "return" values
0093840 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged

gibfahn commented Dec 20, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x-staging and v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Trott mentioned this pull request Dec 20, 2017
Closed
3 tasks

Trott commented Dec 20, 2017

Copy link
Copy Markdown
Member Author

@gibfahn 8.x backport in #17768, assigned to you

Trott added a commit to Trott/io.js that referenced this pull request Dec 20, 2017
test,doc: do not indicate that non-functions "return" values
4c21e2f 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: nodejs#17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Trott mentioned this pull request Dec 20, 2017
Closed
3 tasks

Trott commented Dec 20, 2017

Copy link
Copy Markdown
Member Author

@gibfahn 6.x backport in #17769, assigned to you

Trott added a commit to Trott/io.js that referenced this pull request Dec 20, 2017
test,doc: do not indicate that non-functions "return" values
675b214 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: nodejs#17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test,doc: do not indicate that non-functions "return" values
563f1f6 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Backport-PR-URL: #17768
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
test,doc: do not indicate that non-functions "return" values
c7729eb 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test,doc: do not indicate that non-functions "return" values
9bade7a 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Backport-PR-URL: #17768
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
test,doc: do not indicate that non-functions "return" values
4f6dd96 
Update documentation for test `common` module so that properties that
are not functions do not specify a return type and functions that do
return values specify the type.

PR-URL: #17267
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Jan 24, 2018
Merged
Trott deleted the return-no-return branch January 13, 2022 22:47
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

lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
vsemozhetbyt vsemozhetbyt approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL