Skip to content

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

Merged
Elchi3 merged 6 commits intomdn:masterfrom
ExE-Boss:linter/test-browsers
Jan 10, 2019
Merged

Ensure that some browsers are only allowed in specific categories#2487
Elchi3 merged 6 commits intomdn:masterfrom
ExE-Boss:linter/test-browsers

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

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