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] Added milestone property to GitHub issue details service #7864

Merged
merged 5 commits into from May 21, 2022

Conversation

AlexanderWert
Copy link
Contributor

@AlexanderWert AlexanderWert commented Apr 20, 2022

This PR adds a new property milestone in the GitHub issue details service that shows a badge with the milestone title of an issue if available.

Use case

The purpose of this change is to allow for showing the title of the associated milestone of a GitHub issue (if one is available). In addition, the color of the badge should be derived from the title text, so that milestone badges with the same milestone title have the same color but different color, when the milestone title is different. Therefore, this PR implements picking a color based on the hash of a string.

This feature is very useful when listing issues for tracking purposes. For example when having a list of sub-issues / or related issues, we would like to see at a glance when those issues are planned for execution / related milestone. Here an example based on randomly chosen issues from this repo to demonstrate:

Current workaround

This use case is already possible through a workaround using the dynamic badge service, but it has some limitations:

<img alt="Milestone" src="https://img.shields.io/badge/dynamic/json?label=milestone&query=milestone.title&url=https://api.github.com/repos/badges/shields/issues/4950" align="top"> 

will result in:

Milestone

The limitation of the workaround are:

  • it's cumbersome and actually is a feature that fits very well into the issues details service
  • with the dynamic service the GitHub API is requested unauthorized thus hitting GitHub's rate limit very soon when listing some more issues with corresponding milestone badges at once. This results in a badge like this: image instead of showing the milestone.
  • no color coding based on milestone title

Therefore, I think this feature could be a good addition to the GitHub issue details service and would eliminate the above mentioned limitations.

@calebcartwright
Copy link
Member

calebcartwright commented Apr 23, 2022

Sorry but it's not clear to me what exactly you're proposing, and aspects of the code changes such as the randomization of colors make things even more muddy.

For starters, could you elaborate on the nature of the change from a user/consumer perspective? Is it a badge that displays the name of the milestone an issue is tied to, if the issue is in fact associated with a milestone?

@AlexanderWert
Copy link
Contributor Author

@calebcartwright Thanks for taking this!

I updated the description trying to explain the use case for this feature and the purpose of the color picking a bit better. Please let me know if you have more questions.

@AlexanderWert
Copy link
Contributor Author

Hi @calebcartwright ,

could you give an update on this, please?
Is there something I can further clarify on the use case or should improve on this PR?

Thanks!

@calebcartwright
Copy link
Member

Thank you for the additional detail. However, I still feel like we're trying to do too much here with the colorization. Shields' default's make use of colors in very intentional ways, and we need to remain consistent with those. I understand that you'd like to automatically standardize a colors across the same milestone, but in my opinion that's outside our scope and best left to the badge user.

The associated milestone of a given issue is a purely informational/metadata and as such should have a simple, static informational or blue coloring. Anyone that wants to apply any additional auto-coloring logic as you'd originally proposed can do so via our Custom Endpoint feature

@AlexanderWert
Copy link
Contributor Author

@calebcartwright Thanks for the feedback!
I don't feel strong about the coloring. If apart from the coloring you agree that having a milestone property on the GitHub issue details service is a useful feature, I can just remove the whole coloring part and leave just the core of showing the milestone.

@calebcartwright
Copy link
Member

@calebcartwright Thanks for the feedback! I don't feel strong about the coloring. If apart from the coloring you agree that having a milestone property on the GitHub issue details service is a useful feature, I can just remove the whole coloring part and leave just the core of showing the milestone.

Yup, sorry if that wasn't clear, but definitely onboard with trying to get this incorporated 👍

@AlexanderWert
Copy link
Contributor Author

@calebcartwright Perfect!
I updated the PR by removing the coloring part.

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label May 11, 2022
@shields-ci
Copy link

shields-ci commented May 13, 2022

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

Generated by 🚫 dangerJS against 4e44d1d

@calebcartwright
Copy link
Member

Closed/reopened in the hope that it would unstick CI

Regarding the --- message for issues that aren't added to a milestone, how would you feel about changing that to something else? Though fairly unlikely, it would be possible for someone to create a real milestone using that exact string as the title, and I also think some other approaches might be more consistent.

Part of me feels like we should treat such cases as errors and do our typical throw new NotFound..., but even if not, I think something a little more explicit like no milestone would be better (I realize that could also technically be a real title but the explicitness softens that edge a bit imo)

@calebcartwright
Copy link
Member

I really wish I knew what circumstances cause CI to not run for some people, closing and reopening

@calebcartwright
Copy link
Member

Looks like unit tests are failing can you take a look when time permits? You should be able to run a subset including the failing test via

$ npm run test:core -- --grep GithubIssueDetail

otherwise everything else looks good and we should be all set to merge once the tests are passing

@AlexanderWert
Copy link
Contributor Author

@calebcartwright I fixed the failing unit test

@calebcartwright
Copy link
Member

Service tests are failing, npm run test:services -- --only=GitHubIssueDetail

@AlexanderWert
Copy link
Contributor Author

Sorry, missed that before, the tests should be fixed now.
Tried to close / re-open the PR to trigger the CI. Not sure sure me doing it works or not.

@repo-ranger repo-ranger bot merged commit 5b07779 into badges:master May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants