Skip to content

Conversation

@andrestejerina97
Copy link
Contributor

@andrestejerina97 andrestejerina97 force-pushed the feature/add-badge-formatters branch from b39cb5e to e147182 Compare February 7, 2026 21:47
@andrestejerina97 andrestejerina97 marked this pull request as ready for review February 8, 2026 12:48
@andrestejerina97 andrestejerina97 force-pushed the feature/add-badge-formatters branch 2 times, most recently from 95e3dd9 to 58b90cf Compare February 9, 2026 21:00
@andrestejerina97 andrestejerina97 force-pushed the feature/add-badge-formatters branch from 58b90cf to bc5ed07 Compare February 10, 2026 19:24
@martinquiroga-exo
Copy link
Contributor

@andrestejerina97
NIT: All 4 formatters look exactly the same, perhaps we can abstract them away and just have functions that return the needed strings.
Perhaps something similar to what was done here: #468

Copy link
Contributor

@martinquiroga-exo martinquiroga-exo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Approved.

@martinquiroga-exo I agree that they're virtually identical and it seems wasteful/redundant in a way but it's also a set pattern. It's a good evaluation but not worth rejecting the PR - i.e. your nit vs rejection. Good call. The other PR also was solving for another problem - same entity type with different message formats. This one is the inverse. If we want to address it later we'd need to do so across the board.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants