shields icon indicating copy to clipboard operation
shields copied to clipboard

feat: add [gitlabmergerequests] service

Open sunny0826 opened this issue 2 years ago • 7 comments

re #8077

sunny0826 avatar Jul 05 '22 07:07 sunny0826

Warnings
:warning:

:books: Remember to ensure any changes to config.private in services/gitlab/gitlab-merge-requests.service.js are reflected in the server secrets documentation

Messages
:book: :sparkles: Thanks for your contribution to Shields, @sunny0826!

Generated by :no_entry_sign: dangerJS against 309df894aa2928e1c9589a1b40ed7ee25e4ad18a

shields-ci avatar Jul 05 '22 07:07 shields-ci

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

calebcartwright avatar Jul 05 '22 16:07 calebcartwright

@sunny0826 just want to make sure you saw this :point_right: https://github.com/badges/shields/pull/8166#issuecomment-1175232531

calebcartwright avatar Jul 21 '22 00:07 calebcartwright

@sunny0826 just want to make sure you saw this 👉 #8166 (comment)

I saw this, but wasn't sure what I needed to do.

sunny0826 avatar Jul 21 '22 00:07 sunny0826

@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

calebcartwright avatar Jul 21 '22 02:07 calebcartwright

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!

calebcartwright avatar Jul 22 '22 03:07 calebcartwright

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 separate transform 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.

sunny0826 avatar Jul 22 '22 03:07 sunny0826