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

Meta: appreciate web-platform-tests for normative changes #1850

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 3, 2016

Part of #1849.

@foolip foolip added the do not merge yet Pull request must not be merged per rationale in comment label Oct 3, 2016
@domenic
Copy link
Member

domenic commented Oct 3, 2016

(We've been using "Meta:" instead of "Editorial:" for this sort of thing.)

@foolip foolip changed the title Editorial: require web-platform-tests for normative changes Meta: require web-platform-tests for normative changes Oct 3, 2016
@foolip foolip changed the title Meta: require web-platform-tests for normative changes Meta: ask for web-platform-tests for normative changes Oct 3, 2016
@foolip foolip force-pushed the require-tests branch 3 times, most recently from 795f354 to 86e4c06 Compare October 4, 2016 16:11
@foolip foolip changed the title Meta: ask for web-platform-tests for normative changes Meta: appreciate web-platform-tests for normative changes Oct 4, 2016
@foolip
Copy link
Member Author

foolip commented Oct 4, 2016

Some more tweaking of the wording, can you take a look, @annevk?

@foolip foolip removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 4, 2016
@@ -18,6 +18,10 @@ Please add your name to the Acknowledgements section (search for `<!-- ACKS`) in

To preview your changes locally, follow the instructions in the [html-build repository](https://github.com/whatwg/html-build).

#### Tests

For normative changes, a corresponding [web-platform-tests](https://github.com/w3c/web-platform-tests) PR is highly appreciated. The author and reviewer can be different than for the spec change. If testing is not practical, please explain why and if appropriate file an issue to follow up later.
Copy link
Member

Choose a reason for hiding this comment

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

s/spec change/change to the standard/

Also link "file an issue" to https://github.com/w3c/web-platform-tests/issues/new. LGTM with that, but probably someone else should sign off too given we already had a longer chat about this.

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 also ask that the html label is added, so that we can triage these issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

"different than for the change to the standard" become not very readable, so I'll make it "HTML Standard PR" instead.

@foolip foolip force-pushed the require-tests branch 2 times, most recently from 5bb54a0 to 9cd1a45 Compare October 5, 2016 12:02
@foolip
Copy link
Member Author

foolip commented Oct 5, 2016

I'm happy with the state of this now. Assigning to those I'd like sign off from.

@foolip foolip assigned sideshowbarker, zcorpan and domenic and unassigned annevk Oct 5, 2016
@@ -18,6 +18,10 @@ Please add your name to the Acknowledgements section (search for `<!-- ACKS`) in

To preview your changes locally, follow the instructions in the [html-build repository](https://github.com/whatwg/html-build).

#### Tests

For normative changes, a corresponding [web-platform-tests](https://github.com/w3c/web-platform-tests) PR is highly appreciated. The author and reviewer can be different than for the HTML Standard PR. If testing is not practical, please explain why and if appropriate [file an issue](https://github.com/w3c/web-platform-tests/issues/new) with the `html` label to follow up later.
Copy link
Member

Choose a reason for hiding this comment

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

Not everyone can add labels, so maybe that's not really practical? Though if the issue contains a link to a whatwg/html issue or PR or a link to the spec itself, we can search for "whatwg html".

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, should I just drop the label bit?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe the label bit should be advice in TEAM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to TEAM.md with some more words.

@@ -18,6 +18,10 @@ Please add your name to the Acknowledgements section (search for `<!-- ACKS`) in

To preview your changes locally, follow the instructions in the [html-build repository](https://github.com/whatwg/html-build).

#### Tests

For normative changes, a corresponding [web-platform-tests](https://github.com/w3c/web-platform-tests) PR is highly appreciated. The author and reviewer can be different than for the HTML Standard PR. If testing is not practical, please explain why and if appropriate [file an issue](https://github.com/w3c/web-platform-tests/issues/new) with the `html` label to follow up later.
Copy link
Member

Choose a reason for hiding this comment

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

s/than for/from/?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@foolip foolip merged commit 6408a8f into master Oct 5, 2016
@annevk annevk deleted the require-tests branch October 5, 2016 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants