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

Enforce alphabetical order of browsers in the support block #1882

Merged
merged 5 commits into from
Feb 1, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Apr 21, 2018

Depends on #2889
Fixes #398
Closes #1474; Closes #2890

@ExE-Boss ExE-Boss force-pushed the linter/alphabetical-support-order branch 2 times, most recently from 73f64c6 to e8f5132 Compare April 21, 2018 15:47
@Elchi3 Elchi3 added the linter Issues or pull requests regarding the tests / linter of the JSON files. label Apr 23, 2018
@ExE-Boss ExE-Boss force-pushed the linter/alphabetical-support-order branch from 94b2bdf to feb1101 Compare May 9, 2018 17:44
@ExE-Boss ExE-Boss force-pushed the linter/alphabetical-support-order branch from 3a5aa91 to ddf95bf Compare June 27, 2018 09:30
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jun 27, 2018

@Elchi3 If you’re gonna merge this, you should probably do it now when I just rebased this to master (before all the merge conflicts pile up again like in #1474)

@Elchi3
Copy link
Member

Elchi3 commented Jun 27, 2018

Thanks for working on this!
I don't think it's useful to merge this at this point as it will add rebasing work and additional fixing work to all currently opened PRs. I would rather like to spend my time on getting the PR backlog down now and merge this later. This change can be done programmatically at any time, lets rebase it again (or re-run an alphabetization script again) when we're ready to merge.

@connorshea
Copy link
Contributor

connorshea commented Aug 23, 2018

@ExE-Boss @Elchi3 how about now? :D

Do you have this script still? 🤔

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Aug 23, 2018

Yes, I still have the script I used for this (it’s actually the same script I used for #1712, #2332 and #2333, just run with different paramters).

@Elchi3
Copy link
Member

Elchi3 commented Aug 23, 2018

Yes, we can do this soon. There are other PRs that affect many files, though. Trying to get in #1356 for example, so maybe wait a bit before rebasing.

@connorshea
Copy link
Contributor

577 files left to go! :D

@Elchi3
Copy link
Member

Elchi3 commented Nov 2, 2018

The backlog is relatively small now. I would review a PR that sorts api/*. Anyone interested? @ExE-Boss @vinyldarkscratch @connorshea ?

@queengooborg
Copy link
Contributor

@Elchi3 Ask and ye shall receive. 😉

@Elchi3
Copy link
Member

Elchi3 commented Nov 3, 2018

This needs to be rebased now and then we might be able to finally enforce this :)
Thanks to everyone involved so far! 👍

@connorshea
Copy link
Contributor

😱🥳

@Elchi3
Copy link
Member

Elchi3 commented Nov 8, 2018

Sorry, I didn't see that you pushed a merge here. Latest master has two new issues now unfortunately:

Problems in 2 files:
api/Element.json
  File : api/Element.json
  Style – Error on line #2125
    Actual:               "webview_android": {
    Expected:             "chrome": {
css/properties/justify-content.json
  File : css/properties/justify-content.json
  Style – Error on line #545
    Actual:                 "opera": {
    Expected:               "ie": {

Seeing these messages, I wonder if we should have a better error message. Especially to new contributors it might not be obvious what we want here.

@queengooborg
Copy link
Contributor

Seeing these messages, I wonder if we should have a better error message. Especially to new contributors it might not be obvious what we want here.

+1 -- it's not the most descriptive error message in the world, haha. 😛

@Elchi3
Copy link
Member

Elchi3 commented Jan 31, 2019

I've resolved the conflict here and added a (naive) special case to see how this could look like. It now outputs this against master for me:

Problems in 3 files:
api/Element.json
  File : api/Element.json
  Browser name sorting – Error on line #2128
    Actual:               "webview_android": {
    Expected:             "chrome": {
css/properties/justify-content.json
  File : css/properties/justify-content.json
  Browser name sorting – Error on line #545
    Actual:                 "opera": {
    Expected:               "ie": {
test/sample-data.json
  File : test/sample-data.json
  Browser name sorting – Error on line #8
    Actual:               "webview_android": {
    Expected:             "chrome": {

I think this looks like a better error message. What do you think about this, @ExE-Boss? Do you want to rebase and land this then?

@ExE-Boss
Copy link
Contributor Author

Let’s finally land this.

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.

This should pass on master now. r+
Thanks for your work! 👍

@Elchi3 Elchi3 merged commit ba9026e into mdn:master Feb 1, 2019
@ExE-Boss ExE-Boss deleted the linter/alphabetical-support-order branch February 1, 2019 11:48
@ExE-Boss ExE-Boss mentioned this pull request Feb 4, 2019
3 tasks
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.

4 participants