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: Docs for the common module API in tests #8840

Closed
wants to merge 2 commits into from

Conversation

paulgrock
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Alphabetized list of methods and properties for the common.js
module in test. Add table of contents with link to API doc to the top
of the readme.

Alphabetized list of methods and properties for the common.js module.
Add table of contents to the top of the readme.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 29, 2016
# Tests
# Table of Contents
* [test directories](#tests)
* [common module API](#commonjs)
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 the link text should be consistent with the corresponding title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll change the headers of each section to match the link text here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you capitalize here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to read 'Common module API'.

@mscdex
Copy link
Contributor

mscdex commented Sep 29, 2016

I think all links should be collected at the bottom of the document and referenced appropriately like is done with documentation in doc/api/.

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Sep 29, 2016
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

A lot of these can use some expansion, rewording, etc., but as a first pass to get everything documented minimally, this LGTM.

### tmpDirName
* return [<String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)

Name of the tmpDir, currently 'tmp'
Copy link
Member

@Trott Trott Sep 29, 2016

Choose a reason for hiding this comment

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

It's not always tmp. It is initialized to that, but later on in common.js, it gets altered if the TEST_THREAD_ID environment variable is defined. I would just say: "Name of the temp directory used by tests."

Nit: period at the end for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I'll reword this in the next couple of days.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add a newline at the end of the file. Upcoming linting on .md files will require it. (The file passes all the other lint rules currently configured in .remarkrc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also, I've never seen remark. That's pretty cool

@Trott
Copy link
Member

Trott commented Sep 29, 2016

LGTM with nit about link text. Thank you for doing this!!!

@Trott
Copy link
Member

Trott commented Sep 29, 2016

@nodejs/testing

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I think this is very useful, but we should probably try to format more like the API docs for consistency.

# Tests
# Table of Contents
* [test directories](#tests)
* [common module API](#commonjs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you capitalize here?

@@ -126,3 +130,243 @@ and `setInterval`).
| Runs on CI |
|:----------:|
| No |


## Common.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you lowercase this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to read 'Common module API' to match the wording used in the table of contents. Is that cool?



## Common.js

Copy link
Contributor

Choose a reason for hiding this comment

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

A sentence or two here about what common.js is would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you think of those couple of sentences.

## Common.js

### allowGlobals(...whitelist)
* `whitelist` [<Array>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array) Array of Globals
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't typically use angle brackets around the data types do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

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

LGTM

Update some text and new line at end of README
@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

@cjihrig Have your comments been addressed?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Yea, this is a good start.

@Fishrock123
Copy link
Contributor

Great idea. :D

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 3, 2016
Alphabetized list of methods and properties for the common.js module.
Add table of contents to the top of the readme.

PR-URL: nodejs#8840
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 3, 2016

Landed in 2cfd7fd

@Trott Trott closed this Oct 3, 2016
jasnell pushed a commit that referenced this pull request Oct 6, 2016
Alphabetized list of methods and properties for the common.js module.
Add table of contents to the top of the readme.

PR-URL: #8840
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Alphabetized list of methods and properties for the common.js module.
Add table of contents to the top of the readme.

PR-URL: #8840
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants