[GitHub] Number of commits between branches/tags/commits

[PaulaBarszcz]

Related to: #6901

It returns the number of commits between given branches/tags/commits.

We call under the hood:
https://api.github.com/repos/microsoft/vscode/compare/1.60.0...82f2db7

Example branches names taken from stale branches from:
https://github.com/microsoft/vscode/branches/stale

correct - branches
http://localhost:8080/github/commits-difference/microsoft/vscode?branchA=standalone/0.1.x&branchB=release/1.21

correct - tag...commit
http://localhost:8080/github/commits-difference/microsoft/vscode?branchA=82f2db7&branchB=1.60.0

incorrect - commits
http://localhost:8080/github/commits-difference/microsoft/vscode?branchA=82f2db7&branchB=fffffff

incorrect - missing required query param
http://localhost:8080/github/commits-difference/microsoft/vscode?branchA=82f2db7

[shields-ci]

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

Generated by 🚫 dangerJS against 730fd1e

[PaulaBarszcz]

Following a discussion with @calebcartwright on Discord
I’ll change separating two branches names with …
to one branch name as the namedParam, and the other as the queryParam, as they both can contain slashes.

---

Caleb wrote:

i would actually suggest that we use two separate query parameters in order to cleanly separate the two. for better or worse, branch names can have slashes within them, so trying to account for two variable length separators with route parameters is going to be impossible natively.
you are correct about our general rules pushing for route parameters for everything that's required, but this is a scenario where it's perfectly viable to utilize query parameters and where it would be preferable. trying to use token separators in a single param could get tricky

[PaulaBarszcz]

This PR is ready to be reviewed.

[PaulaBarszcz]

I don’t think that the recent pipeline failed because of the changes introduced in this PR (?)

https://github.com/badges/shields/actions/runs/3055809500/jobs/4929284513

[chris48s]

I don’t think that the recent pipeline failed because of the changes introduced in this PR

Yeah - looks like a transient issue with NPM. They should pass next time you push a commit

[chris48s]

Following a discussion with @calebcartwright on Discord.....

I think the approach of having 2 query params is fine, and we don't need to change it. That said, while I was reading this it occurred to me that another way we could have approached this would have been to take a single string param /:diff+, leave it to the user to call /github/commits-difference/myuser/myrepo/main...feature and just pass the whole diff string to the GH API. Was that the solution you were thinking of to start with?

[chris48s]

OK. This one is a bit easier than the packagist PR 😌 Nearly all of this is already ready to go.

[chris48s]

As is often the case, the hardest job is giving things good names :)

As demonstrated by the fact that the 2 examples you've used here are a commit hash and a tag, branchA and branchB don't actually have to be branches. Fortunately in git there is a word for this identifier that could be a branch name, commit hash or tag name, which is "ref" (you'll also notice you can use the "special" ref names just fine here like HEAD~1, etc if you try them).

The other thing here is that A and B aren't very descriptive either. It isn't really clear to me which one of this is expected to be the ref that is further ahead/behind. The Github API calls these params {base}...{head} which isn't going to be completely obvious to everyone, but I think those terms are more descriptive than A and B and at least have the advantage of maintaining consistency with the terms GitHub uses. Another option is GitLab uses {from}...{to} for these concepts..

So we could go with

• base / head
• base_ref / head_ref
• from / to
• from_ref / to_ref

(we use underscores in query params)

Personally, I have a weak preference for base and head just because they're the same words github uses in the API docs, but if you have a strong preference that one of the other 3 options there is more meaningful we can go with any of those. And lets make the error message "could not establish commit difference between refs"

[PaulaBarszcz]

Personally, I have a weak preference for base and head just because they're the same words github uses in the API docs, but if you have a strong preference that one of the other 3 options there is more meaningful we can go with any of those. And lets make the error message "could not establish commit difference between refs"

Why do we use underscores in query params (and not hyphens or camelCase)?

Sure, I'll rename branchA to base, and branchB to head.

The error message will read "could not establish commit difference between refs".

Yeah, I was also not fond of putting branch in the URL (since users can use a couple of other options) - you're right that ref is a great term to encompass all of the possibilities. Thanks;)

[PaulaBarszcz]

Following a discussion with @calebcartwright on Discord.....

I think the approach of having 2 query params is fine, and we don't need to change it. That said, while I was reading this it occurred to me that another way we could have approached this would have been to take a single string param /:diff+, leave it to the user to call /github/commits-difference/myuser/myrepo/main...feature and just pass the whole diff string to the GH API. Was that the solution you were thinking of to start with?

Yes, at first I wrote the solution with /:diff+ and directly passing the string with ... but Caleb mentioned that trying to use token separators in a single param could get tricky, so I switched to 2 separate query params ;)

[chris48s]

Why do we use underscores in query params (and not hyphens or camelCase)?

This is a good question.

I can tell you that at some point we seem to have (mostly?) consistently settled on underscore for query params e.g:

• shields/services/npm/npm-base.js

  Lines 33 to 35 in 93fa955

  	export const queryParamSchema = Joi.object({
  	registry_uri: optionalUrl,
  	}).required()

• shields/services/gitlab/gitlab-forks.service.js

  Lines 10 to 12 in 93fa955

  	const queryParamSchema = Joi.object({
  	gitlab_url: optionalUrl,
  	}).required()

• shields/services/github/github-release.service.js

  Lines 12 to 14 in 93fa955

  	const extendedQueryParamSchema = Joi.object({
  	display_name: Joi.string().valid('tag', 'release').default('tag'),
  	})

• shields/services/matrix/matrix.service.js

  Lines 4 to 6 in 93fa955

  	const queryParamSchema = Joi.object({
  	server_fqdn: Joi.string().hostname(),
  	}).required()

• and so on

Given the param names get converted to variables, it would be inconvenient to use kebab-case as - isn't valid in a variable name. I'm not really sure exactly how we arrived at snake_case over camelCase for the query params. I guess in general we try to keep the URLs the end-user calls lowercase characters only. I'm not sure I have a 100% satisfying answer though. This may be a pattern that had already emerged before I started contributing to the project.