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

Use the generalized get_pr_latencies for import stats #15

Merged
merged 3 commits into from
Apr 26, 2018

Conversation

foolip
Copy link
Collaborator

@foolip foolip commented Apr 26, 2018

From #10.

Fixes #4.

@foolip
Copy link
Collaborator Author

foolip commented Apr 26, 2018

OK, with 3c4ead3 there are only sub-second differences in what the latency is, as well as 5 PRs missing from the output relative to before.

web-platform-tests/wpt#10445 and web-platform-tests/wpt#10543 are because of web-platform-tests/wpt#10572 (no tags) and will presumably be fixed by adding the tags.

web-platform-tests/wpt#6559 and web-platform-tests/wpt#6845 are Chromium exports that should be excluded and do have a value in the chromium_commit field in wpt-prs.csv.

web-platform-tests/wpt#6449 wasn't really an import, but looks like it from the merge commit.

It looks like the last 3 remain only because the database is read back from disk and wouldn't be output by the script from scratch. This PR drops the caching, so that's OK.

@foolip foolip merged commit 5713ce7 into master Apr 26, 2018
@foolip foolip deleted the simplify-import-stats branch April 26, 2018 09:47
@foolip
Copy link
Collaborator Author

foolip commented Apr 26, 2018

@Hexcles, post-land review appreciated for style, even if I'm confident I didn't regress the data now.

Copy link
Contributor

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

Thanks for double checking that data! Glad to see that:

  1. My previous overly complicated code is confirmed to be correct.
  2. The code is now greatly simplified!


for entry in latencies:
pr = entry['pr']
i = entry['event']
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nitpicking here, but i is not a good variable name here (single-character variable names are more acceptable in lambdas, and i in particular is usually used for iterators).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, import was the original name, that's a keyword, and I didn't know what to call it. Ideas?

una pushed a commit to una/ecosystem-infra-stats that referenced this pull request Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants