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.

Sign up for GitHub

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

Jump to bottom

Linter: don't allow version_added === version_removed #3546

Merged
merged 6 commits into from
Mar 8, 2019
Merged

Linter: don't allow version_added === version_removed #3546

merged 6 commits into from
Mar 8, 2019

Conversation

queengooborg
Copy link
Contributor

Fixes #3531 and all violating data.

@Elchi3 Elchi3 added 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. rebase-needed 🚧 labels Feb 27, 2019
@queengooborg
Copy link
Contributor Author

Rebase complete! Review @Elchi3?

@Elchi3 Elchi3 requested a review from a2sheppy March 1, 2019 14:30
@Elchi3
Copy link
Member

Elchi3 commented Mar 1, 2019

@a2sheppy can you comment on the WebRTC changes here?

@Elchi3 Elchi3 removed the request for review from a2sheppy March 7, 2019 14:50
@Elchi3
Copy link
Member

Elchi3 commented Mar 7, 2019

So, the added/removed nonsense comes from confluence and was submitted in this PR #3287. I think we should change it back to null or maybe to false

@queengooborg
Copy link
Contributor Author

Sounds good to me! I'm leaning towards setting them to null as it seems that the Confluence data for RTCPeerConnection isn't the most trustworthy (as stated in #3287). Changes coming right up!

@Elchi3 Elchi3 added the data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS label Mar 8, 2019
Elchi3
Elchi3 approved these changes Mar 8, 2019
View reviewed changes
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thank you @vinyldarkscratch 👍

I hope this lint will prevent Confluence from submitting weird version added/removed pairs in the future :)

@Elchi3 Elchi3 merged commit cc8404c into mdn:master Mar 8, 2019
@queengooborg queengooborg deleted the fix-3531 branch March 8, 2019 10:52
@queengooborg
Copy link
Contributor Author

💜

ExE-Boss
ExE-Boss reviewed Mar 8, 2019
View reviewed changes
Copy link
Contributor

@ExE-Boss ExE-Boss left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, all of these (especially env being supported as constant) are correct like so:

css/properties/custom-property.json
},
{
"version_added": "11",
"version_removed": "11",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_removed": "11",
"version_removed": "11.1",

css/properties/custom-property.json
},
{
"version_added": "11",
"version_removed": "11",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_removed": "11",
"version_removed": "11.1",

api/RTCPeerConnection.json
@@ -305,8 +305,7 @@
}
],
"safari": {
"version_added": "11",
"version_removed": "11"
"version_added": null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_added": null
"version_added": "11",
"version_removed": "11.1"

api/RTCPeerConnection.json
@@ -1521,8 +1520,7 @@
}
],
"safari": {
"version_added": "11",
"version_removed": "11"
"version_added": null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_added": null
"version_added": "11",
"version_removed": "11.1"

api/RTCPeerConnection.json
@@ -1638,8 +1636,7 @@
}
],
"safari": {
"version_added": "11",
"version_removed": "11"
"version_added": null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_added": null
"version_added": "11",
"version_removed": "11.1"

api/RTCPeerConnection.json
@@ -1890,8 +1887,7 @@
}
],
"safari": {
"version_added": "11",
"version_removed": "11"
"version_added": null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_added": null
"version_added": "11",
"version_removed": "11.1"

api/RTCPeerConnection.json
@@ -3728,8 +3724,7 @@
}
],
"safari": {
"version_added": "11",
"version_removed": "11"
"version_added": null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version_added": null
"version_added": "11",
"version_removed": "11.1"

@queengooborg
Copy link
Contributor Author

queengooborg commented Mar 8, 2019 •
edited
Loading

Ah, good catch @ExE-Boss! I just double-checked with the Confluence data myself (I'm bad at double-checking), and can confirm that the RTCPeerConnection properties were indeed added in 11, removed in 11.1. (I'm having a hard time tracking down the data for custom-property, though.)

@Elchi3
Copy link
Member

Elchi3 commented Mar 8, 2019

I think it would be good to consult a different source as confluence can be wrong also. (I'm always skeptical when confluence claims something was added and removed from one version to another)

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Mar 8, 2019 •
edited
Loading

The constant(…) function alias was (and possibly still is) documented on the official Apple docs (see also #2172)

queengooborg added a commit to queengooborg/browser-compat-data that referenced this pull request Mar 9, 2019
@queengooborg
Revert "Linter: don't allow version_added === version_removed (mdn#3546…
8c0d9be 
…)"

This reverts commit cc8404c.
queengooborg added a commit to queengooborg/browser-compat-data that referenced this pull request Mar 9, 2019
@queengooborg
Revert "Linter: don't allow version_added === version_removed (mdn#3546…
796c778 
…)"

This reverts commit cc8404c.
@queengooborg
Copy link
Contributor Author

queengooborg commented Mar 9, 2019 •
edited
Loading

The WebKit blog, Safari TP Release Notes (see releases 41 and 42), and CanIUse also confirm the existence of constant(), which was then entirely replaced by env(). The release dates of Safari TP 41 and 42 put it around 11.1.

Next is to research RTCPeerConnection and double-check its data!

@queengooborg queengooborg mentioned this pull request Mar 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ExE-Boss ExE-Boss ExE-Boss left review comments

@Elchi3 Elchi3 Elchi3 approved these changes

Assignees
No one assigned
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:css Compat data for CSS features. https://developer.mozilla.org/docs/Web/CSS linter Issues or pull requests regarding the tests / linter of the JSON files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@queengooborg @Elchi3 @ExE-Boss