Skip to content

Migration 005: Order multiple support blocks reverse-chronologically#5352

Merged
queengooborg merged 22 commits intomdn:mainfrom
queengooborg:linter/support-block-ordering
May 31, 2022
Merged

Migration 005: Order multiple support blocks reverse-chronologically#5352
queengooborg merged 22 commits intomdn:mainfrom
queengooborg:linter/support-block-ordering

Conversation

@queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Dec 18, 2019

I was looking around for some projects to work on for BCD while my VM hard drive was down (hardware failure a within a month of purchase...urgh), so I found #1596, which discusses ordering arrays of support blocks ("statements") in reverse chronological order, with a few additional rules. This PR aims to introduce a fix script for just that, creating compare-statements.js, and more importantly, fix-statement-order.js. The function orders all statements with the newest first, within the following groups (in order):

  1. Statements with only version added and/or removed (and potentially notes)
  2. Statements with alternative names or prefixes
  3. Statements with partial support
  4. Statements with flags

A test has been written as well to ensure the function is behaving as expected. Fixes #1596, fixes #7403.

@queengooborg queengooborg requested a review from Elchi3 as a code owner December 18, 2019 11:44
@ghost ghost added the linter Issues or pull requests regarding the tests / linter of the JSON files. label Dec 18, 2019
@queengooborg queengooborg added the bulk_update An update to a mass amount of data, or scripts/linters related to such changes label Dec 18, 2019
@queengooborg queengooborg changed the title Migration: Order multiple support blocks reverse-chronologically Migration 005: Order multiple support blocks reverse-chronologically Dec 18, 2019
@queengooborg queengooborg mentioned this pull request Mar 27, 2020
23 tasks
@ddbeck ddbeck self-assigned this Oct 27, 2020
@queengooborg queengooborg requested a review from ddbeck as a code owner February 13, 2021 01:39
@github-actions github-actions bot added the scripts Issues or pull requests regarding the scripts in scripts/. label Feb 13, 2021
Base automatically changed from master to main March 24, 2021 12:53

/**
*
* Sort a list of compatibility statements based upon reverse-chronological order in the following order:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this grouping is going to make the support blocks un-chronological in some cases. In particular I'm thinking of APIs that were in Presto or Edge, then prefixed-only in Chromium after the switch, and eventually unprefixed again, like this:

"opera": [
{
"version_added": "58"
},
{
"version_added": "15",
"prefix": "webkit"
},
{
"version_added": "12.1",
"version_removed": "15"
}
],

I think the lint should at minimum allow this order, and ideally require it.

I have some guesses, but it would be good to spell out what the downsides would be of sorting all entries by version unconditionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true, the current code will move that prefix statement down to the bottom. However, I'm unsure on how we can allow for that set of statements while disallowing something like this:

"firefox": [
  {
    "version_added": "61",
    "prefix": "-webkit-",
    "notes": "Added prefixed version for backwards compatibility."
  },
  {
    "version_added": "48"
  }
]

Copy link
Contributor

@foolip foolip Apr 22, 2022

Choose a reason for hiding this comment

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

I think we can! First sort chronologically strictly, then find the "best" entry to show by default and move that to the beginning.

Or we move this kind of logic to consumers, making this a much bigger project...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely think we should handle the logic here in BCD so that our consumers have less to worry about.

May I pass this PR off to you to implement the logic you recommended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we actually hold off on implementing this logic for another PR/migration? While I think this would be great for us to do, I'm just not sure how to put it into code yet, and I'd love to get the order resolved so that our consumers aren't confused by the lack of order...

@queengooborg queengooborg requested a review from caugner May 24, 2022 04:21
partial_implementation: true,
notes: 'No fries with the burger',
},
{ version_added: '20', prefix: 'webkit' },
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit surprising to me that this active support item comes after a chronologicaly older inactive (version_removed) item. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that prefixes would be moved further down in the list, but I see what you mean -- let me update this!

queengooborg and others added 2 commits May 24, 2022 20:00
@queengooborg queengooborg requested a review from caugner May 25, 2022 17:56
@queengooborg
Copy link
Contributor Author

queengooborg commented May 31, 2022

I'm self-merging this because the main reviewer for infra PRs is OOO and review comments have been addressed. We can follow up with any sorting changes in the future.

@queengooborg queengooborg merged commit d294502 into mdn:main May 31, 2022
@queengooborg queengooborg deleted the linter/support-block-ordering branch May 31, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bulk_update An update to a mass amount of data, or scripts/linters related to such changes linter Issues or pull requests regarding the tests / linter of the JSON files. scripts Issues or pull requests regarding the scripts in scripts/.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linearize support version info Linter: Make sure multiple version ranges are sorted according to some rules

5 participants