shields
shields copied to clipboard
feat: add [gitlabmergerequests] service
re #8077
Warnings | |
---|---|
:warning: |
:books: Remember to ensure any changes to |
Messages | |
---|---|
:book: | :sparkles: Thanks for your contribution to Shields, @sunny0826! |
Generated by :no_entry_sign: dangerJS against 309df894aa2928e1c9589a1b40ed7ee25e4ad18a
Have only done a quick pass through this but a couple early notes:
- given the 10k ceiling, that seems like a case we should handle explicitly in our rendering, even if it requires a special case handling
- consider if/how the handle/fetch/transform function paradigm could be applied with this class
@sunny0826 just want to make sure you saw this :point_right: https://github.com/badges/shields/pull/8166#issuecomment-1175232531
@sunny0826 just want to make sure you saw this 👉 #8166 (comment)
I saw this, but wasn't sure what I needed to do.
@sunny0826 just want to make sure you saw this point_right #8166 (comment)
I saw this, but wasn't sure what I needed to do.
Ah okay, no worries! In the future don't hesitate to ask questions in these types of cases, especially if something is unclear. I'll follow up tomorrow with some elaboration on my prior comment so we can figure out next steps
After looking through a little more closely, I see that you are already applying special case handling to the >10k scenario which is good :+1: (I was just emphasizing that we should in my prior comment).
The second part of my comment was really just related to the length of some of those methods, and a suggestion to consider using the transform
function to encapsulate some concerns and hopefully shorten some other functions. However, I'm not sure how much that will really buy given the nature of what's going on in those two other functions. I'll try to leave some inline feedback over the weekend as I think there's some other changes that can make those a little shorter, and we can reconsider if a separate transform
function is still worthwhile afterwards.
Finally, thanks again for all your work on these GitLab badges, it's very much appreciated!
After looking through a little more closely, I see that you are already applying special case handling to the >10k scenario which is good 👍 (I was just emphasizing that we should in my prior comment).
The second part of my comment was really just related to the length of some of those methods, and a suggestion to consider using the
transform
function to encapsulate some concerns and hopefully shorten some other functions. However, I'm not sure how much that will really buy given the nature of what's going on in those two other functions. I'll try to leave some inline feedback over the weekend as I think there's some other changes that can make those a little shorter, and we can reconsider if a separatetransform
function is still worthwhile afterwards.Finally, thanks again for all your work on these GitLab badges, it's very much appreciated!
Thank you for your feedback.