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

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

Merged
merged 22 commits into from
May 31, 2022

Conversation

queengooborg
Copy link
Collaborator

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

@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 added this to Linter improvements in Non-data issue overview Mar 27, 2020
@queengooborg queengooborg mentioned this pull request Mar 27, 2020
23 tasks
@queengooborg queengooborg added this to Low Priority in Vinyl's refactoring project May 15, 2020
@ddbeck ddbeck added this to Tooling and broad data quality improvements in Prioritization review Sep 4, 2020
@ddbeck ddbeck self-assigned this Oct 27, 2020
@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
scripts/compare-statements.js Outdated Show resolved Hide resolved

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

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
Collaborator 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
Collaborator

@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
Collaborator 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
Collaborator 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
scripts/lib/compare-statements.js Outdated Show resolved Hide resolved
scripts/lib/compare-statements.js Outdated Show resolved Hide resolved
scripts/lib/compare-statements.js Outdated Show resolved Hide resolved
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
Collaborator 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 queengooborg requested a review from caugner May 25, 2022 17:56
@queengooborg
Copy link
Collaborator 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
Prioritization review automation moved this from Tooling and broad data quality improvements to Done 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
No open projects
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