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

Ensure that some browsers are only allowed in specific categories #2487

Merged
merged 6 commits into from
Jan 10, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jul 17, 2018

This has been suggested by @connorshea in #2332 (comment), and I agree that it’s a good idea, as it ensures that contributors don’t try to add firefox_ios or thunderbird to non‑WebExtension entries, or nodejs to non‑JavaScript entries (which it turns out they already did).

It also ensures that PRs like #2484 get better error messages.


review?(@a2sheppy, @Elchi3)

@ExE-Boss ExE-Boss mentioned this pull request Jul 17, 2018
test/test-browsers.js Outdated Show resolved Hide resolved
@ddbeck ddbeck added the linter Issues or pull requests regarding the tests / linter of the JSON files. label Jul 18, 2018
@Elchi3
Copy link
Member

Elchi3 commented Jan 8, 2019

I think this is no longer relevant now. In #3160 we've seen that node implements some features in api/ land and so that validation isn't really useful anymore. For the other edge case browsers, we have decided to not add them to the repo.

I could imagine to test for webextension-only browsers, but I'm not sure if it is really neeeded. What do you think, @ExE-Boss ?

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 8, 2019

I think it might be useful.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Okay cool, then I would like to reduce the scope of this to only test for WebExtension browsers.

@ExE-Boss ExE-Boss force-pushed the linter/test-browsers branch 2 times, most recently from fec9edc to 683e8c5 Compare January 8, 2019 13:21
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 8, 2019

The way I understand it, the goal for MDN is to only display Node in the api category when it’s present, so I removed the nodejs keys from HTML, Audio and Media entries.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

The nodejs removals are fine with me, thanks.
I have a few suggestions to the new test, but generally it is nicely done. Thanks for your work 👍
Please see my comments inline.

test/test-browsers.js Outdated Show resolved Hide resolved
test/test-browsers.js Outdated Show resolved Hide resolved
test/test-browsers.js Outdated Show resolved Hide resolved
test/test-browsers.js Outdated Show resolved Hide resolved
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good to me now. 👍

@Elchi3 Elchi3 merged commit 21f5f45 into mdn:master Jan 10, 2019
@Elchi3 Elchi3 changed the title Ensure that edge‑case browsers are only allowed in specific categories Ensure that some browsers are only allowed in specific categories Jan 10, 2019
@ExE-Boss ExE-Boss deleted the linter/test-browsers branch January 10, 2019 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter Issues or pull requests regarding the tests / linter of the JSON files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants