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 browser order in compatibility tables [MERGE #2889 FIRST] #2890

Closed
wants to merge 1 commit into from

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented Oct 1, 2018

This PR is intended to enforce an alphabetical order in __compat.support, as originally proposed by #1474. Fixes #398, closes #1474. This is one of two PRs that supersedes #1474, whereas the other one (#2889) sorts the browser compatibility order.

NOTE: The Travis test will fail until #2889 is merged.

Warning: make sure to merge #2889 first before merging this one, else Travis tests will always fail.
Warning: Existing PRs will need to be updated to adhere to this new rule.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 1, 2018

I already do this in #1882.

Copy link
Contributor

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

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

This PR has numerous problems, and #1882 should be preferred for the actual validation.

@@ -2,6 +2,26 @@
const fs = require('fs');
const path = require('path');

function orderSupportBlock(name, val) {
if (name == 'support') {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ WARNING! ⚠️ This will break any value named support, eg. test.support:

"test": {
  "support": {
    "property": {},
    "method": {}
  }
}

would be made invalid, whereas:

"test": {
  "other": {
    "property": {},
    "method": {}
  }
}

would remain valid.

@@ -245,4 +245,4 @@
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, stop breaking the last newline.

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 honestly don't know why the newline keeps trying to get removed. It might be the JSON editor I'm using (which is out of date anyways). :/

@@ -2,6 +2,26 @@
const fs = require('fs');
const path = require('path');

function orderSupportBlock(name, val) {
if (name == 'support') {
Copy link
Contributor

@ExE-Boss ExE-Boss Oct 1, 2018

Choose a reason for hiding this comment

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

Also, wrong indentation.

@queengooborg
Copy link
Contributor Author

Ah, I didn't see that you had combined the test and data updates into one PR, my bad, @ExE-Boss -- looking closer into your changes, it seems that you have a better function than what was written in #1474 (which I literally just copied and pasted, since the test and the data updates were merged into one commit, thus requiring a rebase which created the possibility of old data replacing the new). Since you're marking the other PR (with the actual data updates) as a dependency for your PR, perhaps you'd be up for reverting the changes to the data files and only keeping the test script updates, effectively maintaining the same structure as this PR, or merging our efforts into one single PR?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Oct 1, 2018

I plan on reverting the data change commit and only keeping the test case update commit (and leaving the data update for #2889).

@queengooborg
Copy link
Contributor Author

Sounds good!

@queengooborg queengooborg deleted the enforce-browser-order branch October 2, 2018 16:48
@queengooborg queengooborg added duplicate Duplicate issues or pull requests. This one is closed in favor of the other issue or pull request. invalid Invalid issues or pull requests (wrong repo, spam, duplicates, etc.). This won't get merged. Sorry! linter Issues or pull requests regarding the tests / linter of the JSON files. labels Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Duplicate issues or pull requests. This one is closed in favor of the other issue or pull request. invalid Invalid issues or pull requests (wrong repo, spam, duplicates, etc.). This won't get merged. Sorry! 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.

lint.js should check browser order
2 participants