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 008: add script for removing redundant flags #6822

Closed

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Oct 4, 2020

This PR adds a migration script to go along with the new irrelevant flag removal guidelines added in #6670 by doing the following:

  • Removes flag data in features added by default over two years ago
  • Removes flag data for flags removed over two years ago

Note: since the migration script is time-based, the test script will probably fail in the future. I didn't want to introduce a new dependency just to mock the time, however.

@queengooborg queengooborg requested a review from ddbeck as a code owner October 4, 2020 15:30
@github-actions github-actions bot added 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/. labels Oct 4, 2020
@ddbeck
Copy link
Collaborator

ddbeck commented Oct 5, 2020

I've got some bad news about this PR, @vinyldarkscratch. 😬

I don't see our guideline, as written, sustaining a change like this. The guideline has two qualifiers that I think preclude doing a migration for this kind of data:

The removal of the support statement must not create an ambiguous gap or void in the data for that browser

These conditions represent minimum requirements for the removal of valid flag data; other considerations may result in flag data continuing to be relevant, even after the guideline conditions are met.

The first might be solvable (though I'm not sure we have a robust definition of "ambiguous" data), but the second definitely isn't—and that's on purpose. I might've been more explicit about my intent when initially proposing the guideline in #6670, but I didn't actually want to sanction automated removal just yet. I had a few motives:

  • I didn't want to do any large-scale changes to the data that would be difficult to undo (see Interim project goals #6738). A migration like this would become quickly unrevertable.
  • Removing old flags may lead to cascading removals (e.g., if a flag removal resulted in an all-false feature, then we'd also need to remove that feature, depending on its development status). En masse, this might be disruptive to consumers (necessitating a version increment). It would also be a lot of work that I didn't want to take on while also managing the mdn- to @mdn/ migration.
  • I wanted to facilitate the removal of dubious data or data that would be difficult to reconcile, not scrub our data set of old flags generally. High-quality historic flag data may still be of interest to spec authors and the like and I didn't want to foreclose on that audience.

These considerations, taken together, point toward closing the PR. I'm annoyed at myself for letting you put in all this work for little gain. To avoid this in the future, we should probably begin to follow our own advice more closely and use issues to declare intent on this sort of thing before implementing it (I've been an offender too, on this count).

I'm sorry to have to close this, especially knowing that you've put some work into this. I am very grateful that you've put in the effort here and that you've given me an opportunity to be more clear about the aims of the guideline. Thank you.

@ddbeck ddbeck closed this Oct 5, 2020
@queengooborg
Copy link
Contributor Author

I totally understand, no worries! I'll keep this code around if we ever want to look at it again in the future. 😉

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.

2 participants