refactor [GitHubCommitActivity] to switch to v4/GraphQL API #6959

calebcartwright · 2021-08-28T02:38:56Z

Resolves #2704

shields-ci · 2021-08-28T02:39:43Z

	Messages
📖	✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against 960fa14

calebcartwright · 2021-08-28T02:39:44Z

services/github/github-commit-activity.service.js

  }

  static examples = [
    {
      title: 'GitHub commit activity',
      // Override the pattern to omit the deprecated interval "4w".
-      pattern: ':interval(y|m|w)/:user/:repo',
+      pattern: ':interval(y|m|w)/:user/:repo/',


was debating whether to add another example that showed the ability to specify a branch/ref but that ties in to a broader question asked below

calebcartwright · 2021-08-28T02:42:48Z

services/github/github-commit-activity.service.js

+          $since: GitTimestamp!
+        ) {
+          repository(owner: $user, name: $repo) {
+            object(expression: $branch) {


Best I can tell this value is used in a call to git's rev-list directly or indirectly via rev-parse and is a required part of the query. My understanding is that defaulting to this to HEAD means it'll work with the default branch in the target repo regardless of what that branch is named, and seems like a good idea given we must provide something.

One thing I wasn't clear on was whether the existing v3 API did the same internally or if it spanned across all the branches. At the moment the v3 is still "down" (HTTP 202 - processing) so I can't really compare the results to check for any differences. I think if we go this route with this type of query then it makes sense to surface up the ref that's used here as a route parameter so that users can specify their preference.

(edit: switched to HEAD over FETCH_HEAD for default)

The docs are slightly unclear on this, but https://docs.github.com/en/rest/reference/repos#statistics says the stats match the graphs in the front end (e.g: https://github.com/badges/shields/graphs/contributors ). My interpretation is they do only show commits to the default branch. I think this assumption is good

calebcartwright · 2021-08-28T02:49:35Z

services/github/github-commit-activity.service.js

+    if (interval === 'y') {
+      now.setUTCFullYear(now.getUTCFullYear() - 1)
+    } else if (interval === 'm' || interval === '4w') {
+      now.setUTCDate(now.getUTCDate() - 30)


The classic fun question of "what is a month, really?" 😆 I know we still have momentjs in our dep tree but I opted to avoid using it to prevent from increasing our dependence on it due to #5573.

IMO showing the commit activity for the last 30 days is a reasonable time period to cover both of these intervals, but suppose we could try to do something more complex. I feel like some of the complexity we had before was more due to the nature of the v3 API response that just gave us total counts in weekly slices, but lmk if folks have any concerns with treating month as the last 30 days

There are other badges where we are making the same assumption e.g:

shields/services/packagecontrol/packagecontrol.service.js

Lines 56 to 71 in b02471f

month: {

base: 'packagecontrol/dm',

messageSuffix: '/month',

transform: resp => {

const platforms = resp.installs.daily.data

let downloads = 0

platforms.forEach(platform => {

// total for the first 30 days

for (let i = 0; i < 30; i++) {

downloads += platform.totals[i]

}

})

return downloads

},

name: 'PackageControlDownloadsMonth',

},

shields/services/sourceforge/sourceforge.service.js

Lines 22 to 24 in b02471f

dm: {

startDate: endDate => moment(endDate).subtract(30, 'days'),

suffix: '/month',

shields/services/homebrew/homebrew-downloads.service.js

Lines 19 to 23 in b02471f

const periodMap = {

dm: {

api_field: '30d',

suffix: '/month',

},

shields/services/wordpress/wordpress-downloads.service.js

Lines 31 to 34 in b02471f

dm: {

limit: 30,

messageSuffix: '/month',

},

I don't know where it fits in the docs, but I'd be happy to document somewhere that if the upstream api doesn't explicitly provide stats for "month", we assume month = 30 days and formalise this.

Cool, glad to be in good company!

calebcartwright · 2021-08-28T14:56:22Z

Discovered an issue poking around the review app, can't seem to convert the PR back to a draft but working on it

chris48s · 2021-08-28T15:06:35Z

For reference, you should have a button under "reviewers"

calebcartwright · 2021-08-28T15:10:04Z

For reference, you should have a button under "reviewers"

TIL 😄

calebcartwright · 2021-08-28T15:27:28Z

Comparisons and commentary between the activity shown with this vs. current below. One key difference between the v4 results vs the v3 results is that v4 is currently using an exact date and time window, vs. whole day rounding, so there are slightly different windows at the date boundaries

weekly

https://img.shields.io/github/commit-activity/w/badges/shields

https://shields-staging-pr-6959.herokuapp.com/github/commit-activity/w/badges/shields/

These happen to produce the same value (at the moment anyway), but that's largely just a coincidence since our current badge displays the total commits from the prior full week which is something we had to do to account for the structure of the v3 API response to avoid only showing 2-3 days worth of commits on some days earlier in the week.

Monthly

In addition to the start-of-day delta, there's also the 30 day window in this approach vs. the calculated month estimate we had before

eslint

https://img.shields.io/github/commit-activity/m/eslint/eslint

https://shields-staging-pr-6959.herokuapp.com/github/commit-activity/m/eslint/eslint

shields

https://img.shields.io/github/commit-activity/m/badges/shields

https://shields-staging-pr-6959.herokuapp.com/github/commit-activity/m/badges/shields

Yearly

eslint

https://img.shields.io/github/commit-activity/y/eslint/eslint

https://shields-staging-pr-6959.herokuapp.com/github/commit-activity/y/eslint/eslint

shields

https://img.shields.io/github/commit-activity/y/badges/shields

https://shields-staging-pr-6959.herokuapp.com/github/commit-activity/y/badges/shields

chris48s

I think the core point of this badge is to give a ballpark of how active a repo is rather than to display a precise value. Consistently working with very slightly different assumptions (which might yield slightly different numbers for some users) is better than broken for some users.

calebcartwright · 2021-08-29T17:51:51Z

I think the core point of this badge is to give a ballpark of how active a repo is rather than to display a precise value. Consistently working with very slightly different assumptions (which might yield slightly different numbers for some users) is better than broken for some users.

Yup completely agreed on this and the documentation front as well. If it starts popping up as a common question/source of confusion then I'll clarify, perhaps as a Discussion topic or somewhere in our markdown docs, as well as inline docs in the badge builder window.

looks like the github commit activity badges have been fixed: badges/shields#6959

calebcartwright added service-badge New or updated service badge keep-service-tests-green Related to fixing failing tests of the services labels Aug 28, 2021

shields-cd temporarily deployed to shields-staging-pr-6959 August 28, 2021 02:39 Inactive

calebcartwright commented Aug 28, 2021

View reviewed changes

calebcartwright force-pushed the gh-commit-activity-to-v4 branch from 69c6e10 to 38d6bf6 Compare August 28, 2021 15:11

calebcartwright temporarily deployed to shields-staging-pr-6959 August 28, 2021 15:11 Inactive

refactor(GitHubCommitActivity): switch to v4/GraphQL API

644d0f8

calebcartwright force-pushed the gh-commit-activity-to-v4 branch from 38d6bf6 to 644d0f8 Compare August 28, 2021 15:28

calebcartwright temporarily deployed to shields-staging-pr-6959 August 28, 2021 15:29 Inactive

chris48s approved these changes Aug 29, 2021

View reviewed changes

calebcartwright added the squash when passing label Aug 29, 2021

Merge branch 'master' into gh-commit-activity-to-v4

960fa14

shields-cd deployed to shields-staging-pr-6959 August 29, 2021 17:52 View deployment

repo-ranger bot merged commit 76ca283 into master Aug 29, 2021

repo-ranger bot deleted the gh-commit-activity-to-v4 branch August 29, 2021 17:59

acusti added a commit to acusti/uikit that referenced this pull request Sep 26, 2021

📝 Update “last commit” badge → commits/week

74f61ba

looks like the github commit activity badges have been fixed: badges/shields#6959

acusti added a commit to acusti/uikit that referenced this pull request Sep 26, 2021

📝 Update “last commit” badge → commits/week

8f6daaa

looks like the github commit activity badges have been fixed: badges/shields#6959

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor [GitHubCommitActivity] to switch to v4/GraphQL API #6959

refactor [GitHubCommitActivity] to switch to v4/GraphQL API #6959

calebcartwright commented Aug 28, 2021

shields-ci commented Aug 28, 2021 •

edited

Loading

calebcartwright Aug 28, 2021

calebcartwright Aug 28, 2021 •

edited

Loading

chris48s Aug 29, 2021

calebcartwright Aug 28, 2021

chris48s Aug 29, 2021

calebcartwright Aug 29, 2021

calebcartwright commented Aug 28, 2021

chris48s commented Aug 28, 2021

calebcartwright commented Aug 28, 2021

calebcartwright commented Aug 28, 2021 •

edited

Loading

chris48s left a comment

calebcartwright commented Aug 29, 2021 •

edited

Loading

	month: {
	base: 'packagecontrol/dm',
	messageSuffix: '/month',
	transform: resp => {
	const platforms = resp.installs.daily.data
	let downloads = 0
	platforms.forEach(platform => {
	// total for the first 30 days
	for (let i = 0; i < 30; i++) {
	downloads += platform.totals[i]
	}
	})
	return downloads
	},
	name: 'PackageControlDownloadsMonth',
	},

	dm: {
	startDate: endDate => moment(endDate).subtract(30, 'days'),
	suffix: '/month',

	const periodMap = {
	dm: {
	api_field: '30d',
	suffix: '/month',
	},

refactor [GitHubCommitActivity] to switch to v4/GraphQL API #6959

refactor [GitHubCommitActivity] to switch to v4/GraphQL API #6959

Conversation

calebcartwright commented Aug 28, 2021

shields-ci commented Aug 28, 2021 • edited Loading

calebcartwright Aug 28, 2021

Choose a reason for hiding this comment

calebcartwright Aug 28, 2021 • edited Loading

Choose a reason for hiding this comment

chris48s Aug 29, 2021

Choose a reason for hiding this comment

calebcartwright Aug 28, 2021

Choose a reason for hiding this comment

chris48s Aug 29, 2021

Choose a reason for hiding this comment

calebcartwright Aug 29, 2021

Choose a reason for hiding this comment

calebcartwright commented Aug 28, 2021

chris48s commented Aug 28, 2021

calebcartwright commented Aug 28, 2021

calebcartwright commented Aug 28, 2021 • edited Loading

weekly

Monthly

eslint

shields

Yearly

eslint

shields

chris48s left a comment

Choose a reason for hiding this comment

calebcartwright commented Aug 29, 2021 • edited Loading

shields-ci commented Aug 28, 2021 •

edited

Loading

calebcartwright Aug 28, 2021 •

edited

Loading

calebcartwright commented Aug 28, 2021 •

edited

Loading

calebcartwright commented Aug 29, 2021 •

edited

Loading