Skip to content

doc: add common.WPT to test README - #11127

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:wpt
Closed

doc: add common.WPT to test README#11127
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:wpt

Conversation

@Trott

@TrottTrott commented Feb 2, 2017

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

doc test

@TrottTrott added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Feb 2, 2017
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 2, 2017
@Trott

Trott commented Feb 2, 2017

Copy link
Copy Markdown
MemberAuthor

(This documentation is a stub.)

@TrottTrott mentioned this pull request Feb 2, 2017
2 tasks

@TimothyGuTimothyGu 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.

Other than the nit, LGTM.

Comment threadtest/README.md Outdated

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.

A link would be helpful (Web Platform Tests and/or testharness.js). Also the second sentence seems to have an extra "in".

@TimothyGu

Copy link
Copy Markdown
Member

I personally don't really think it's necessary to extend this documentation stub, when WPT's APIs are documented elsewhere anyway.

@Trott

Trott commented Feb 2, 2017

Copy link
Copy Markdown
MemberAuthor

@TimothyGu Thanks. I incorporated both links and got a little more specific with the text. PTAL.

@TimothyGuTimothyGu 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.

LGTM. Thanks for writing this.

Comment threadtest/README.md Outdated

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.

I would say it's more like a "mock" or "port"? But this description LGTM too.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

"port" works for me. I'll change it.

@mhdawsonmhdawson 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.

LGTM

@Trott
Trottforce-pushed the wpt branch 2 times, most recently from d28ea8c to c284e24CompareFebruary 2, 2017 23:18
Comment threadtest/README.md Outdated

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.

Maybe ### WPT (Web Platform Tests) for people who don't know what it is and don't see the link two lines down?

@TrottTrottFeb 2, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Parentheses are used to show arguments in the other entries so I probably wouldn't do it exactly that way. But if there's a way to clarify it in the text below, I'm happy to.

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.

Maybe just start the next line with it, something like:

Web Platform Tests - A port of parts of

Comment threadtest/README.md Outdated

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

👍 Done.

Trott added a commit to Trott/io.js that referenced this pull request Feb 6, 2017
PR-URL: nodejs#11127
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

Trott commented Feb 6, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in 3fffebb

@TrottTrott closed this Feb 6, 2017
italoacasas pushed a commit that referenced this pull request Feb 6, 2017
PR-URL: #11127
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
PR-URL: nodejs#11127
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
PR-URL: nodejs#11127
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@Trott@TimothyGu@jasnell@thefourtheye@joyeecheung@mhdawson@gibfahn@nodejs-github-bot