mdn / browser-compat-data

This repository contains compatibility data for Web technologies as displayed on MDN
https://developer.mozilla.org
Creative Commons Zero v1.0 Universal
4.86k stars 1.95k forks source link

Multiple statements with flags #16598

Open gsnedders opened 2 years ago

gsnedders commented 2 years ago

I managed to accidentally have the following in a PR:

"safari": [
  {
    "version_added": "16"
  },
  {
    "version_added": "preview",
    "flags": [
      {
        "type": "preference",
        "name": "CSS overflow: clip support"
      }
    ]
  }
],

This is somewhat clearly nonsense.

Per our logic, preview > 16, and it doesn't make sense for it to be added in 16, and also added in a later version behind a flag without being removed from the earlier version.

This should also be raising an error due to the ordering, given preview > 16.

gsnedders commented 2 years ago

Actually, looking at what the linter does, the origin of this issue isn't really about preview at all, as the following is equally accepted, contrary to #16287:

"safari": [
  {
    "version_added": "16"
  },
  {
    "version_added": "17",
    "flags": [
      {
        "type": "preference",
        "name": "CSS overflow: clip support"
      }
    ]
  }
],
Elchi3 commented 8 months ago

https://github.com/mdn/browser-compat-data/pull/17005 added a lint for this but decided to ignore flags, so your above statements still pass in BCD today. We can debate if that's a good choice or not.

If you remove d.flags from this condition: https://github.com/mdn/browser-compat-data/blob/35bbaad7f1085e4d90de7941ffcf8a1f7aca5624/test/linter/test-multiple-statements.ts#L29

your above examples will fail. In existing data, you'll only get very few failures in the CI:

Multiple Statements - 6 problems (6 errors, 0 warnings):
 ✖ api.ScreenOrientation.lock - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.
 ✖ css.at-rules.font-face.src.tech_keyword - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.
 ✖ css.properties.content-visibility - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.
 ✖ css.properties.offset-path.ray - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.
 ✖ css.types.ray.size - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.
 ✖ http.headers.Service-Worker-Navigation-Preload - Error → firefox has multiple statements for normal name exist without partial implementation! Please merge these statements and combine their notes, or set partial_implementation to true on applicable statements.

So, we could investigate if these 6 are non-sense or if they actually make sense.