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

Add missing Edge 12 data based on Confluence #3392

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

foolip
Copy link
Contributor

@foolip foolip commented Feb 5, 2019

Produced by npm run confluence -- --fill-only --browsers=edge with
local changes applied to only include changes where the new version
was 12.

The data was collected manually from Edge 12 in an exotic VM:
mdittmer/web-apis#75
mdittmer/web-apis#74

Also discussed in #3309.

@foolip
Copy link
Contributor Author

foolip commented Feb 5, 2019

I've pushed one more commit that also updates existing data where it seems the APIs shipped earlier than BCD says.

@Elchi3 I feel the changes in this PR are likely to be correct, since Edge 12 was the first release of Edge, and this all means the APIs were found on the prototype chain. There is a chance it makes some data inconsistent if there are others parts that couldn't be updated, but this is still in the direction of accuracy.

@Elchi3 Elchi3 added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 6, 2019
@foolip
Copy link
Contributor Author

foolip commented Feb 6, 2019

Produced by `npm run confluence -- --fill-only --browsers=edge` with
local changes applied to only include changes where the new version
was 12.

The data was collected manually from Edge 12 in an exotic VM:
mdittmer/web-apis#75
mdittmer/web-apis#74
@foolip foolip force-pushed the confluence-edge-12 branch from c8787a3 to ba317be Compare February 8, 2019 11:45
@foolip
Copy link
Contributor Author

foolip commented Feb 8, 2019

I've resolved conflicts and rebased, dropping addition commits to just replace null and true values with no manual fixes in this PR. Should make it easier to review I hope.

@foolip
Copy link
Contributor Author

foolip commented Feb 13, 2019

@Elchi3 is any more evidence for this PR needed?

@Elchi3
Copy link
Member

Elchi3 commented Feb 13, 2019

I don't think so. I wanted to do 5-10 spot checks here and have a glance over all changes, but I just haven't had the time yet. I will try to come back to this once I'm back next week. Also happy if someone else wants to have look here meanwhile.

@foolip
Copy link
Contributor Author

foolip commented Feb 14, 2019

I could do the manual spot checks in my Edge 12 VM if that would be convincing enough.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 14, 2019

I'll spot check this today, @foolip (ideally, if there are no issues, it can land in the release I'm doing today).

@foolip
Copy link
Contributor Author

foolip commented Feb 14, 2019

Thanks @ddbeck! Let me know if anythings seems suspect and I'll check it out. The only kind of false positives I think are plausible here is if the APIs were exposed but they actually didn't work yet.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 14, 2019

OK, I'm satisfied here (even had independent confirmation of one entry with #3424, how about that) and I'm going to merge it. Thank you, @foolip!

@ddbeck ddbeck merged commit 5af6fe6 into mdn:master Feb 14, 2019
@foolip foolip deleted the confluence-edge-12 branch February 14, 2019 15:25
@foolip
Copy link
Contributor Author

foolip commented Feb 14, 2019

Thanks all! I'll see about what more data I can scrape out of Confluence with fairly high confidence :)

@Elchi3
Copy link
Member

Elchi3 commented Feb 15, 2019

Thank you very much @foolip 👍

We like to set an OKR around the data quality and so we're measuring true and null values for our success with this effort. Edge is a browser we want to measure as part of this. I'm happy to see a clear improvement to the data, see these statistics (fyi, the stats exclude webextension data):

BCD 0.0.66

edge-data-0-0 66

BCD 0.0.67 (after this PR and others)

edge-data-0-0-67

@Elchi3
Copy link
Member

Elchi3 commented Feb 15, 2019

I think we've landed PRs that added Edge versions 12 to 18, but we haven't done a PR that fills in where we think Edge support would be false. Is that correct?

foolip added a commit to foolip/browser-compat-data that referenced this pull request Feb 15, 2019
Produced by `npm run confluence -- --fill-only --browsers=edge` with
local changes applied to only include changes where the new version
was 13.
@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2019

@Elchi3 I'm very glad to hear this making a dent! Is the script you're using to measure this public somewhere? I'd love to see it change over time!

I think we've landed PRs that added Edge versions 12 to 18, but we haven't done a PR that fills in where we think Edge support would be false. Is that correct?

Right, the Confluence approach can't be trusted for absence of support in the same way as it can be trusted for support. It looks like npm run confluence only makes changes based on transitions in support too, so even where Confluence has non-support for Edge 12-18 the tool isn't doing anything. It should be pretty easy to tweak it, but the output would require a lot of manual review, so I'd rather focus on improving the approach.

Looks like I forgot to send a PR for Edge 13 too: #3461

@Elchi3
Copy link
Member

Elchi3 commented Feb 15, 2019

@Elchi3 I'm very glad to hear this making a dent! Is the script you're using to measure this public somewhere? I'd love to see it change over time!

Right now it is a spreadsheet that @atopal created a year ago. It would be nice to create a dashboard from it. I'm using 0.0.66 now as the baseline to measure our success for the year. https://docs.google.com/spreadsheets/d/1bJGXeoq-2bDxe2S2X9PRjhWPuISaJzQi1XWZ2D9lFDY/edit#gid=590936194 (script for data collecting https://github.com/atopal/browser-compat-data-dashboard)

Right, the Confluence approach can't be trusted for absence of support in the same way as it can be trusted for support. It looks like npm run confluence only makes changes based on transitions in support too, so even where Confluence has non-support for Edge 12-18 the tool isn't doing anything. It should be pretty easy to tweak it, but the output would require a lot of manual review, so I'd rather focus on improving the approach.

Yeah, I think the version numbers we got from confluence in this PR and others were really great and helped a lot, but in order to get the percentage of real values really high now, we need to think about how to get correct false values as well.

@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2019

We could change null to false based on Confluence and it would probably be >80% correct, but unless we vet each case carefully, some errors will slip through. Does someone have the patience to review that if it's a large PR? I wouldn't, so https://github.com/foolip/mdn-bcd-collector is my current attempt to improve the approach.

@Elchi3
Copy link
Member

Elchi3 commented Feb 15, 2019

Yeah, large PRs would be a bit rough, smaller ones might work and then we distribute the reviewing some more, but I'm looking forward to seeing what comes out of your new project. Thanks again for your work on this!

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 15, 2019

Seconding smaller PRs. And for these sorts of automated PRs, it would help to include, in the PR, a brief rundown of how they ought to be reviewed so it's not necessarily dependent on Florian and me to look at them. For example, if we said, "Reviewers: check 10% of the features for consistency with Confluence and three features for external corroboration" (or whatever) and that'd make it lot easier for drive by reviews to happen.

@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2019

@ddbeck sure, I can suggest a method for spot checking in future PRs. But when suggesting my own standards by which to review something, asking someone else to do the review seems sort of redundant. In #3461 I self-reviewed to the point where someone else can rubberstamp without really looking.

@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2019

consistency with Confluence

For my "based on Confluence" PRs that isn't necessary. It's all verifying and probably finding some number of mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants