Enforce feature order #3427

queengooborg · 2019-02-11T00:51:21Z

This closes #264 and introduces the sorting of all the features by alphabetical order (case insensitive). This also provides a script that automatically sorts said features (as well as the browser order in __compat), which can be called using npm run fix.

Elchi3 · 2019-05-10T15:12:23Z

Again, the devil is in the details :) I think we need to talk a bit how we want to sort special characters.

The characters allowed for the identifiers that we want to sort here are a-zA-Z_0-9-$@ (see the schema file). I think should decide where to put things that start with a number, $, @, _ or -. I think with the sorting you propose here, these would be at the top.
The "__compat" entry should always the first item if it is present and then I think I would rather have the rest at the bottom.

You can observe this for example on this table https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#Browser_compatibility. There you have @@match, and so on, which are currently at the end, but normal sorting would put then at the start.

Another case is https://developer.mozilla.org/en-US/docs/Web/CSS/@media#Browser_compatibility where all the -moz- prefixed items etc would now be at the top when we right now have placed them at the end.

So, I've tried to change the sorting and here's what I came up with to put special chars last (change in fix-feature-order.js).

value = Object.keys(value).sort((a, b) => {
      if(a === '__compat') return -1;
      return (b.toLowerCase().localeCompare("a") >= 0) - (a.toLowerCase().localeCompare("a") >= 0)
             || a.toLowerCase().localeCompare(b.toLowerCase());
    })

I didn't change it in the linter to see the difference to the current sorting, so here's the output of that and where this differs exactly from your original sorting.

Problems in 24 files:
✖ api/CSSNumericArray.json
  Style – 1 error:
      Feature sorting error on line #53
    Actual:         "CSSNumericValue": {
    Expected:       "@@iterator": {
✖ api/CSSTransformValue.json
  Style – 1 error:
      Feature sorting error on line #53
    Actual:         "CSSTransformValue": {
    Expected:       "@@iterator": {
✖ api/CSSUnparsedValue.json
  Style – 1 error:
      Feature sorting error on line #53
    Actual:         "CSSTransformValue": {
    Expected:       "@@iterator": {
✖ api/HTMLCanvasElement.json
  Style – 1 error:
      Feature sorting error on line #156
    Actual:           "bitmaprenderer_context": {
    Expected:         "2d_alpha_context": {
✖ api/OffscreenCanvas.json
  Style – 1 error:
      Feature sorting error on line #133
    Actual:           "bitmaprenderer_context": {
    Expected:         "2d_context": {
✖ api/StylePropertyMapReadOnly.json
  Style – 1 error:
      Feature sorting error on line #53
    Actual:         "entries": {
    Expected:       "@@iterator": {
✖ css/at-rules/media.json
  Style – 1 error:
      Feature sorting error on line #55
    Actual:           "any-hover": {
    Expected:         "-moz-device-pixel-ratio": {
✖ css/properties/background-position.json
  Style – 1 error:
      Feature sorting error on line #54
    Actual:           "multiple_backgrounds": {
    Expected:         "4_value_syntax": {
✖ css/properties/background-repeat.json
  Style – 1 error:
      Feature sorting error on line #54
    Actual:           "multiple_backgrounds": {
    Expected:         "2-value": {
✖ css/properties/border-radius.json
  Style – 1 error:
      Feature sorting error on line #101
    Actual:           "elliptical_borders": {
    Expected:         "4_values_for_4_corners": {
✖ css/types/length-percentage.json
  Style – 1 error:
      Feature sorting error on line #55
    Actual:           "cap": {
    Expected:         "1in_is_96px": {
✖ css/types/length.json
  Style – 1 error:
      Feature sorting error on line #55
    Actual:           "cap": {
    Expected:         "1in_is_96px": {
✖ javascript/builtins/Array.json
  Style – 1 error:
      Feature sorting error on line #58
    Actual:           "concat": {
    Expected:         "@@iterator": {
✖ javascript/builtins/ArrayBuffer.json
  Style – 1 error:
      Feature sorting error on line #58
    Actual:           "byteLength": {
    Expected:         "@@species": {
✖ javascript/builtins/Date.json
  Style – 1 error:
      Feature sorting error on line #59
    Actual:           "getDate": {
    Expected:         "@@toPrimitive": {
✖ javascript/builtins/Map.json
  Style – 1 error:
      Feature sorting error on line #69
    Actual:           "clear": {
    Expected:         "@@iterator": {
✖ javascript/builtins/RegExp.json
  Style – 1 error:
      Feature sorting error on line #58
    Actual:           "compile": {
    Expected:         "@@match": {
✖ javascript/builtins/Set.json
  Style – 1 error:
      Feature sorting error on line #69
    Actual:           "add": {
    Expected:         "@@iterator": {
✖ javascript/builtins/String.json
  Style – 1 error:
      Feature sorting error on line #58
    Actual:           "anchor": {
    Expected:         "@@iterator": {
✖ javascript/builtins/Symbol.json
  Style – 1 error:
      Feature sorting error on line #59
    Actual:           "asyncIterator": {
    Expected:         "@@toPrimitive": {
✖ javascript/builtins/TypedArray.json
  Style – 1 error:
      Feature sorting error on line #58
    Actual:           "buffer": {
    Expected:         "@@iterator": {
✖ javascript/functions.json
  Style – 1 error:
      Feature sorting error on line #111
    Actual:           "callee": {
    Expected:         "@@iterator": {
✖ webextensions/api/devtools.json
  Style – 1 error:
      Feature sorting error on line #27
    Actual:               "inspect": {
    Expected:             "$0": {
✖ webextensions/manifest/commands.json
  Style – 1 error:
      Feature sorting error on line #25
    Actual:           "F1-F12": {
    Expected:         "_execute_sidebar_action": {

Long story short: I don't know how you or everyone else feels about this, but I think we should have special char things like the @@match etc. at the bottom.

Elchi3 · 2019-05-10T15:23:44Z

@wbamberg since you've been involved in the sorting issue (#264), would you mind commenting on how to sort special characters? (see my suggestion above)

queengooborg · 2019-05-10T17:17:01Z

I'm in favor of putting them towards the bottom, especially if that's how we've got it currently sorted. I went ahead and committed those changes to orderFeatures() in both the fix and test scripts, so they're ready to go -- I'll re-run the fix scripts on the PRs (and make some new ones for the existing folders)!

wbamberg · 2019-05-10T18:14:32Z

I think the main reason for sorting features is to help people find things, and give them the simplest, most consistent and most predictable rule for finding things.

So for me the answer to "how should we sort special characters?" is "how would most people expect special characters to be sorted?". Personally, I'd expect non-alpha characters to come first, just because that's the default in sort(), so it's a kind of lowest common denominator.

But I don't feel strongly about this. The main things are to have the check in the linter and the fix command.

I saw this is using localeCompare() - I don't know much about locale handling in JavaScript, but does this mean it might give different results in different environments?

queengooborg · 2019-05-10T18:28:55Z

Ah, unfortunately it looks like it may be giving different results. I just gave it a quick test run by merging all of the sorting PRs into a new branch, along with the latest master, and it looks like there's over a thousand files that are being changed from the new code.

We should probably look further into what exactly localeCompare() does.

Elchi3 · 2019-05-10T18:34:31Z

Thanks, Will!

"how would most people expect special characters to be sorted?".

Yes, this question was driving my thinking as well. I believe for the two examples I've given, my view would be that people would expect special things like @@match or -webkit-animation would come last in the table, as everything else seems to be much more important and less obscure.

Given this seems to only affect sorting in 24 files, I think we could go with the "special characters last" approach for now, but change to standard "special characters first" sort, if it causes problems (I don't think it will).

I saw this is using localeCompare() - I don't know much about locale handling in JavaScript, but does this mean it might give different results in different environments?

Good question. I think nodejs now supports internationalization, so this might matter.

queengooborg · 2019-07-02T21:21:34Z

And done, all sorting PRs have been updated to reflect this as well! (I realized I made a slight mistake the first time -- I set it up so that anything with numbers in it is sorted last. Heh, whoops...)

ddbeck · 2019-07-03T14:09:49Z

@vinyldarkscratch A heads up here, I'm not certain I'll have enough time to review this this week (I saw you tagged me in a review, I'm not ignoring it). If I can't get to this week, I'll give you some sort of update on Monday. Thank you!

queengooborg · 2019-07-03T17:54:56Z

Not a problem, thanks for taking the time to review it! There’s definitely no rush. 😉

ddbeck · 2019-07-11T09:16:40Z

Hey @vinyldarkscratch, thanks for your patience on this. Would you be open to sort of rebooting this process, to try out some of the things we've discussed on #4469? I can draw up some next steps for that, if you're interested. Or if you'd rather press on with how things have started here, we can leave the #4469 to another issue. Let me know what you'd prefer.

queengooborg · 2019-07-11T09:28:41Z

We've been referring to this PR in the discussion in #4469. Yeah, let's give this one a reboot with the new plans! What would you like me to do to get this prepared for the new process?

ddbeck · 2019-07-11T11:10:38Z

OK, supposing we were starting from square one, here's how I'd imagine migrations would look:

New migrations would go in a dedicated directory, probably /scripts/migrations/ (e.g., move your fix-it script to /scripts/migrations/sort-features.js)
A migration script itself would take some JSON files as arguments. For instance, I could run node ./scripts/migrations/sort-features.js css/** to fix all of CSS, or run it against a specific JSON file to see what happens. Hopefully this would change the files in place.
A migration script would come with some test case(s) to demonstrate the behavior. I haven't given this a ton of thought. Something as simple as a test input file and an expected output file to diff against could work. But I'd also be OK with introducing something like Jest as a dev dependency, if that would make things more readable and maintainable—then you could have a tidy unit test for the compare function, for example.

As it pertains to this PR specifically, I'd expect:

Other PRs related to this to close.
If you want to include the linter in this PR, then I'd expect that the invocation would be commented out or gated in some way (e.g., setting environment variable). In other words, merging this PR shouldn't cause CI failures.

As you can tell, I'm making this up as I go, so I'm open to ideas or improvements on this.

queengooborg · 2019-07-12T08:59:48Z

Sounds like a plan to me! Regarding the location of the script, personally, I feel that the fix-up script should remain where it is for two reasons:

We're expecting the sorting script to be something that's run more than just one during this bulk change. As such, I feel it should be convenient and accessible by a quick npm run lint.
A refactor I plan to do is for both the fix script and its linter to import common functions and not use copy and paste. Keeping them in the same folder should help with that process.

I'm not stubborn on this. We can easily set up a fix script that uses the scripts within that folder! Which do you think is more appropriate?

ddbeck · 2019-07-15T09:40:05Z

We're expecting the sorting script to be something that's run more than just one during this bulk change

This is a great point and I didn't think of it. Yeah, leave the script where it is.

queengooborg · 2019-08-27T19:40:50Z

The moment has arrived -- this linter is now ready to be merged! Thanks @Elchi3 and @ddbeck! 💜

Elchi3

Thanks for all your work on this one! Looks good to me! Let's merge this now so we don't regress sorting in new PRs anymore.

queengooborg · 2019-08-28T09:31:13Z

Woo, it feels great!

Elchi3 added the linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. label Feb 11, 2019

Elchi3 mentioned this pull request Feb 11, 2019

Add WebSpeech events #3420

Merged

ddbeck added the not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Feb 12, 2019

queengooborg mentioned this pull request Mar 27, 2019

Lint: add version consistency check #3368

Merged

queengooborg added this to Linter improvements in Non-data issue overview Apr 12, 2019

queengooborg requested a review from Elchi3 as a code owner May 5, 2019 22:07

queengooborg added rebase-needed 🚧 and removed rebase-needed 🚧 labels May 10, 2019

queengooborg added 2 commits June 22, 2019 16:44

Add feature sorting

6fd3446

Add fix script

95b7550

queengooborg added 2 commits July 3, 2019 23:22

Fix regex to allow underscores and hyphens when not at start of string

df0e422

Merge branch 'master' into linter/sort-features

065d0ee

ddbeck mentioned this pull request Jul 8, 2019

Formalize a process for bulk changes #4469

Closed

3 tasks

queengooborg added 3 commits July 10, 2019 11:56

Sort single-character features and features with underscores in name

96394d5

Merge branch 'master' into linter/sort-features

994e678

Use new chalk templating

e9bb385

queengooborg added 3 commits July 11, 2019 02:58

Improve regex

1bf4da8

Fix whitespace

61f1abb

Sort entries with symbols within them as well

7c9a601

queengooborg mentioned this pull request Jul 15, 2019

Add fix script to sort browsers and features #4490

Merged

Separate fix script into another PR

ba513cb

queengooborg added this to the Bulk Update: Alphabetical Ordering milestone Jul 15, 2019

queengooborg added the bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes label Aug 14, 2019

queengooborg mentioned this pull request Aug 22, 2019

Enforce browser sorting #4665

Merged

queengooborg removed the not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Aug 23, 2019

queengooborg added 2 commits August 24, 2019 15:56

Merge branch 'master' into linter/sort-features

da9554d

Merge branch 'master' into linter/sort-features

7ac3baf

Elchi3 approved these changes Aug 28, 2019

View reviewed changes

Elchi3 merged commit a7b5216 into mdn:master Aug 28, 2019

queengooborg deleted the linter/sort-features branch August 28, 2019 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce feature order #3427

Enforce feature order #3427

queengooborg commented Feb 11, 2019 •

edited

Loading

Elchi3 commented May 10, 2019

Elchi3 commented May 10, 2019

queengooborg commented May 10, 2019 •

edited

Loading

wbamberg commented May 10, 2019

queengooborg commented May 10, 2019

Elchi3 commented May 10, 2019

queengooborg commented Jul 2, 2019 •

edited

Loading

ddbeck commented Jul 3, 2019

queengooborg commented Jul 3, 2019

ddbeck commented Jul 11, 2019

queengooborg commented Jul 11, 2019

ddbeck commented Jul 11, 2019

queengooborg commented Jul 12, 2019

ddbeck commented Jul 15, 2019

queengooborg commented Aug 27, 2019

Elchi3 left a comment

queengooborg commented Aug 28, 2019

Enforce feature order #3427

Enforce feature order #3427

Conversation

queengooborg commented Feb 11, 2019 • edited Loading

Elchi3 commented May 10, 2019

Elchi3 commented May 10, 2019

queengooborg commented May 10, 2019 • edited Loading

wbamberg commented May 10, 2019

queengooborg commented May 10, 2019

Elchi3 commented May 10, 2019

queengooborg commented Jul 2, 2019 • edited Loading

ddbeck commented Jul 3, 2019

queengooborg commented Jul 3, 2019

ddbeck commented Jul 11, 2019

queengooborg commented Jul 11, 2019

ddbeck commented Jul 11, 2019

queengooborg commented Jul 12, 2019

ddbeck commented Jul 15, 2019

queengooborg commented Aug 27, 2019

Elchi3 left a comment

Choose a reason for hiding this comment

queengooborg commented Aug 28, 2019

queengooborg commented Feb 11, 2019 •

edited

Loading

queengooborg commented May 10, 2019 •

edited

Loading

queengooborg commented Jul 2, 2019 •

edited

Loading