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

[GitHub] Revision badge #2224

Closed
wants to merge 4 commits into from
Closed

[GitHub] Revision badge #2224

wants to merge 4 commits into from

Conversation

Ang-YC
Copy link
Contributor

@Ang-YC Ang-YC commented Oct 28, 2018

This pull request is related to issue #244

Endpoint /github/revision/{username}/{repo}/{path}.svg
Regex /^\/github\/revision\/([^/]+)\/([^/]+)\/(.*)\.(svg|png|gif|jpg|json)$/
Example /github/revision/badges/shields/doc/TUTORIAL.md.svg
Result revision

@shields-ci
Copy link

Messages
📖

✨ Thanks for your contribution to Shields, @Ang-YC!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

@Ang-YC
Copy link
Contributor Author

Ang-YC commented Oct 29, 2018

@RedSparr0w @paulmelnikow Can anyone trigger the CI until it passed? The request keep timeout and I don't know what should I do. Thanks!

@RedSparr0w
Copy link
Member

Thanks for the PR, this badge would be a nice addition to the current GitHub badges.

I've managed to get the CI to pass 👍

Unfortunately I don't have much time to do a full review right now, but from a quick look over, it seems you are using the legacy service class, if possible would you be up for using the new classes?

We have recently updated the tutorial, which would also be great for us to get some more feedback on 😄

@chris48s
Copy link
Member

I'm not keen to accept more legacy code, but the GitHub badges are more complicated to migrate away from the legacy badge format than our other badges because they need to make use of the rotating token pool. I suspect it might be too involved to ask a first-time contributor to do that refactoring for us. If we just write this naively as a standalone badge, we're going to have problems with API usage limits.

How about we either:

  • Put this PR on hold until a regular contributor has done the more heavy-lifting to extract a BaseGitHubService class (or whatever) that can be used by GitHub badges
    or
  • Review this PR as as it stands

What do you think?

@Ang-YC - I wouldn't want you to spend more time on this until we've got clear advice for you. Perhaps you could bear with us while we work it out :)

@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Oct 30, 2018
@Ang-YC
Copy link
Contributor Author

Ang-YC commented Oct 30, 2018

@chris48s I don't mind any decision on this PR, perhaps other members have better idea on the Github migration process from legacy code. (I would like to work on another issue as well and will tag it separately to ask for more clarification)

Thanks @RedSparr0w, I do read the TUTORIAL.md and follows on some of them because I didn't realise the code is legacy until I can't find Github badges that used non-legacy code.

@paulmelnikow
Copy link
Member

I'm good with:

Review this PR as as it stands

👍

@paulmelnikow
Copy link
Member

Given the review hasn't happened and the GitHub service rewrite is underway (#2320 #2253) I think this should be rewritten before it's merged. It feels like time to stop accepting new services under the legacy framework.

If anyone would like to pick this up, or @Ang-YC you decide you want to take it on, feel free to reference this PR when you reopen.

@paulmelnikow paulmelnikow added the on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned label Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants