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 (linter): Enforce version order in linter/test-versions #7399

Closed
wants to merge 1 commit into from

Conversation

saschanaz
Copy link
Contributor

A checklist to help your pull request get merged faster:

  • Summarize your changes
  • Data: link to resources that verify support information (such as browser's docs, changelogs, source control, bug trackers, and tests)
  • Data: if you tested something, describe how you tested with details like browser and version
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!)
  • Link to related issues or pull requests, if any

Fixes #1596

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Nov 18, 2020
@queengooborg queengooborg changed the title Enforce version order in linter/test-versions Migration 005 (linter): Enforce version order in linter/test-versions Nov 18, 2020
@queengooborg queengooborg added the not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Nov 18, 2020
@queengooborg
Copy link
Collaborator

Let's wait on #5352, which includes the migration code to clean up the data first? Since this is such a big change, we should stick with the migration process.

@saschanaz
Copy link
Contributor Author

Yeah, I found there is a prior work, didn't see that 😔. Fortunately not a complete duplicate, though.

@queengooborg
Copy link
Collaborator

Couple of other things as well:

  • Can we revert the changes to the data itself so that we are only keeping the linter changes? It's a bit challenging to see the changes to the linter itself, not to mention that we will be updating the data in a separate, bulk update PR. (I know this will fail CI, but as Florian has said, it's better for it to fail and show that it's working!)
  • We should use the same function for both the linter and the migration/fix script. That way, we're not including two different methods for the same end goal, thus removing redundant lines of code and maintaining consistency.

@github-actions github-actions bot added the linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. label Nov 18, 2020
@ddbeck ddbeck removed their request for review December 17, 2020 16:26
Base automatically changed from master to main March 24, 2021 12:54
@foolip
Copy link
Collaborator

foolip commented Apr 20, 2022

@saschanaz you might want to weigh in on #5352 as well. The precise rules for what order we require are pretty nuanced, and I think will require a bit of back and forth to get right.

* @param {VersionValue} xver
* @param {VersionValue} yver
*/
function compareVersionNumber(xver, yver) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need to handle "preview" as well, which didn't exist when you started this lint. That also makes "number" in the name inaccurate.

*/
function toArray(version) {
if (typeof version === 'string') {
return (version.startsWith('≤') ? version.slice(1) : version).split('.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should use compare-versions to compare versions (with "≤" removed first).

* @param {string} relPath
* @param {Logger} logger
*/
function checkVersionOrder(supportDataList, relPath, browser, logger) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are cases where this lint will require the wrong thing. For example here:

"firefox": [
{
"version_added": "34"
},
{
"version_added": "44",
"alternative_name": "webkitMatchesSelector"
},
{
"version_added": "3.6",
"alternative_name": "mozMatchesSelector",
"notes": [
"Before Firefox 4, invalid selector strings caused false to be returned instead of throwing an exception.",
"See <a href='https://bugzil.la/1119718'>bug 1119718</a> for removal."
]
}
],

The webkitMatchesSelector entry is the newest, but we don't want it to be the first one listed. Or maybe we should want that, but then we need to change the rendering rules of MDN, since that will use the first entry.

@saschanaz
Copy link
Contributor Author

Hmm, now that I'm less active here, I'll drop this in favor of #5352.

@saschanaz saschanaz closed this May 8, 2022
@saschanaz saschanaz deleted the linter-versions-order branch May 8, 2022 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linter: Make sure multiple version ranges are sorted according to some rules
3 participants