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

tidy up [GitHubGist] routes #8510

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Oct 9, 2022

We've merged a couple of PRs recently adding badges for GitHub gists.
Both of these made quite different decisions about what route to use. In particular, the stars one could have caused a conflict. Fortunately, you can't create a github username or team called "gists" 😌 . Bit of sloppy code review on my part there.

In this PR, I propose that the route we should use for github gist badges is

/github/gist/NOUN/PARAMETERS with gist being singular

and then I've updated the 2 existing badges + added redirects.

I'm open to feedback on that. If we think that /github-gist, /github-gists or /github/gists would be better as a base than /github/gist I am happy to update the PR. Whatever we decide, lets: pick one, use it consistently and continue to do so moving forwards.

The other thing I've done in this PR is moved the gist badges to a subdir. The /github dir already has a lot of files in it, so I think it is useful to compartmentalise any further gist badges.

@chris48s chris48s added the service-badge New or updated service badge label Oct 9, 2022
@shields-ci
Copy link

shields-ci commented Oct 9, 2022

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

Generated by 🚫 dangerJS against 7117248

@calebcartwright
Copy link
Member

Unfortunately we didn't have much discussion on this during the original PR, but I'd attempted to tee up the same topic in #8272 (comment)

I don't personally have strong opinions either way, though I know in the past others have expressed more rigorous preferences for having a singular noun in the route. The most important element for me is consistency within the "family" (gists in this case), but also trying to be cognizant of consistency across the service offering.

@chris48s
Copy link
Member Author

Yeah agreed. I care more about establishing the pattern and using it moving forwards than exactly which variant we go with.

Just thinking it through a bit more:

  • Nouns in routes are singular if there is one of the thing: There's only one gist
  • GitHub brand the service as "GitHub Gist" (not "GitHub Gists")

Screenshot at 2022-10-12 20-45-18

which strengthens the case for singular.

The first part of the route should be the service. Gists are kind of an odd feature of GitHub, but I think I'd fundamentally characterise gists as a feature of GitHub, rather than a separate service. For example, we get the data from the GitHub API. I think that leads me back to /github/gist as the base.

@PaulaBarszcz
Copy link
Collaborator

This PR seems helpful and complete - can we merge it or does it still need something before being ready for a merge? :)

@repo-ranger repo-ranger bot merged commit c8a0a86 into badges:master Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants