Mirror script combines statements that shouldn't be

[foolip]

Using the data for createPeriodicWave as an example:

https://github.com/mdn/browser-compat-data/blob/499d5fa0371c2919eb3caf43bc11b2805ae99772/api/BaseAudioContext.json#L643-L660

If one changes the chrome_android statement to "mirror", the built data.json has this data:

            "chrome": [
              {
                "notes": "Default values supported",
                "version_added": "59"
              },
              {
                "version_added": "30"
              }
            ],
            "chrome_android": {
              "notes": "Default values supported",
              "version_added": "30"
            },

In other words, the two statements have been combined, and notes just copied. This isn't correct, these two statements shouldn't be combined.

[foolip]

@queengooborg I discovered this as another bug that prevents using "mirror" in more places. I don't really understand what the overall purpose of combineStatements is, it's not documented or (unit) tested.

It seems to me that the correct behavior should be to map the versions of each statement individually. When that results in the same version_added and version_removed, filter that one out. If there are no statements left, that's just false.

[foolip]

Ah, I think I see what the purpose of this is. If there are two statements with different notes, and those map to the same vesrion_added, then those make sense to combine into a single statement with two notes. combineNotes does something like this.

[queengooborg]

combineStatements was mostly designed for mirroring from multiple sources, ex. mirroring IE + Chrome to Edge. It is used now to remove duplicate statements from statement arrays, as well as strip out any version_added: false statements from an array. Since we've abandoned mirroring from multiple sources, the logic to combine similar statements together is no longer relevant, so we can probably rewrite the function and simplify its logic.

However, I feel that the statements for api.BaseAudioContext.createPeriodicWave are over-complicated and should be combined anyways. If there is a behavioral change in a browser that is significant enough for a note, but not significant enough for partial_implementation, we should not create a separate support statement. In other words, we should replace this:

[ 
   { 
     "version_added": "59", 
     "notes": "Default values supported" 
   }, 
   { 
     "version_added": "30" 
   } 
 ],

...with this:

 { 
   "version_added": "30", 
   "notes": "Before Chrome 59, all arguments must be specified." 
 }

[foolip]

However, I feel that the statements for api.BaseAudioContext.createPeriodicWave are over-complicated and should be combined anyways.

That may be, but there must be legit cases with this structure. Let's leave this one alone until the bug is fixed.

[queengooborg]

Could you provide an example of said case? I can't think of one where partial_implementation isn't involved.

[foolip]

I'm not sure I have a truly good case for this, but we have plenty of data that is affected by this. By hacking up the mirroring script to see what things look like if we don't combine statements, here is existing data that could be mirrored if not for this issue:

https://github.com/mdn/browser-compat-data/blob/30da4ca864ec7480f3c6b153729300cd1432543f/api/Clients.json#L224-L240

https://github.com/mdn/browser-compat-data/blob/30da4ca864ec7480f3c6b153729300cd1432543f/api/CustomElementRegistry.json#L75-L84

https://github.com/mdn/browser-compat-data/blob/30da4ca864ec7480f3c6b153729300cd1432543f/api/WebSocket.json#L647-L667

https://github.com/mdn/browser-compat-data/blob/30da4ca864ec7480f3c6b153729300cd1432543f/api/Window.json#L4105-L4119

https://github.com/mdn/browser-compat-data/blob/30da4ca864ec7480f3c6b153729300cd1432543f/api/_globals/atob.json#L18-L26

https://github.com/mdn/browser-compat-data/blob/36ccf5283411c3244e55a6389df687260b95b9af/css/properties/grid-template-columns.json#L219-L235

https://github.com/mdn/browser-compat-data/blob/36ccf5283411c3244e55a6389df687260b95b9af/css/selectors/descendant.json#L10-L18

https://github.com/mdn/browser-compat-data/blob/36ccf5283411c3244e55a6389df687260b95b9af/css/types/basic-shape.json#L190-L219

https://github.com/mdn/browser-compat-data/blob/36ccf5283411c3244e55a6389df687260b95b9af/html/global_attributes.json#L92-L107

[foolip]

I've also found that we do the wrong thing with prefix and alternative_name. The value of those properties are ignored, and we combine them anyway. This is one such case:

https://github.com/mdn/browser-compat-data/blob/36ccf5283411c3244e55a6389df687260b95b9af/api/Document.json#L940-L953

[queengooborg]

Since we no longer combine any statements, this is now done!