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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 23, 2017

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 23, 2017
vsemozhetbyt
vsemozhetbyt approved these changes Nov 23, 2017
View reviewed changes
test/common/README.md Outdated
[SeCreateSymbolicLinkPrivilege](https://msdn.microsoft.com/en-us/library/windows/desktop/bb530716(v=vs.85).aspx)).
On non-Windows platforms, this currently returns true.
### canCreateSymLink()
*return [<Boolean>]
Copy link
Contributor

Choose a reason for hiding this comment

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

A space after the *

test/common/README.md Outdated
*return [<Boolean>]

Checks whether the current running process can create symlinks. On Windows, this
returns false if the process running doesn't have privileges to create symlinks
Copy link
Contributor

Choose a reason for hiding this comment

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

false -> `false`?

test/common/README.md Outdated
Checks whether the current running process can create symlinks. On Windows, this
returns false if the process running doesn't have privileges to create symlinks
([SeCreateSymbolicLinkPrivilege](https://msdn.microsoft.com/en-us/library/windows/desktop/bb530716(v=vs.85).aspx)).
On non-Windows platforms, this always returns true.
Copy link
Contributor

Choose a reason for hiding this comment

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

true -> `true`?

test/common/README.md Outdated
* return [<Boolean>]

Checks `hasIntl` and `small-icu` is supported.
Indicates `hasIntl` and `small-icu` is supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

are supported?

test/common/README.md Outdated
* return [<Number>] default = `12346`

Port tests are running on.
A port number for tests to use if one is needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with @vsemozhetbyt's comments addressed.

@Trott
Copy link
Member Author

Trott commented Nov 29, 2017

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

@Trott
Copy link
Member Author

Trott commented Nov 29, 2017

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

gibfahn
gibfahn reviewed Nov 29, 2017
View reviewed changes
test/common/README.md
@@ -63,9 +65,7 @@ failures.
Platform normalizes the `dd` command

### enoughTestMem
* return [<Boolean>]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow the convention in our docs and restore these lines but remove the word return.

@Trott
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
Copy link
Member Author

Trott commented Dec 1, 2017

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
@Trott
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
Copy link
Member Author

Trott commented Dec 1, 2017

Landed in dd629ef

@Trott Trott closed this Dec 1, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@Trott @MylesBorins
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
@Trott @MylesBorins
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 MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

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 Trott mentioned this pull request Dec 20, 2017
Closed
3 tasks
@Trott
Copy link
Member Author

Trott commented Dec 20, 2017

@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
@Trott
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 Trott mentioned this pull request Dec 20, 2017
Closed
3 tasks
@Trott
Copy link
Member Author

Trott commented Dec 20, 2017

@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
@Trott
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
@Trott @gibfahn
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
@Trott @MylesBorins
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
@Trott @gibfahn
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 gibfahn mentioned this pull request Dec 20, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 2, 2018
@Trott @MylesBorins
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 MylesBorins mentioned this pull request Jan 24, 2018
Merged
@Trott Trott deleted the return-no-return branch January 13, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@gibfahn gibfahn gibfahn approved these changes

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
@Trott @gibfahn @lpinca @cjihrig @vsemozhetbyt @nodejs-github-bot