-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: support PrintTextf
and PrintErrorf
on Reporter
#706
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
==========================================
- Coverage 78.88% 78.62% -0.27%
==========================================
Files 85 85
Lines 6035 6055 +20
==========================================
Hits 4761 4761
- Misses 1068 1088 +20
Partials 206 206 ☔ View full report in Codecov by Sentry. |
@@ -67,6 +67,9 @@ linters-settings: | |||
|
|||
issues: | |||
exclude-rules: | |||
- path: pkg/reporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes made the reporters big enough to trigger dupl
- I've just disabled it for the whole package because while it's technically not wrong, the only way I know that we could address that would be by having a standardReporter
that gets used by each struct with the basic "print to stdout and error" behaviour (like what I've done in osv-detector
with the "memory database"), but from what I understand that would be a breaking change (because when initializing the struct you have to pass in the inner struct) and it won't actually result in much savings
(because they're code that we just don't have tests for, and adding them would be a lot of work for this PR 😅) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@another-rex let me know if you have thoughts about removing |
I'm thinking we remove them now since it's breaking anyway, and we'll want to have the changes discussed in #698 to be added before we make a release to keep all the breaking changes in one release. |
When I originally implemented
Reporter
, IDEs such as GoLand didn't support customPrintf
functions so I stuck with plain methods and did thefmt
formatting on the string; that's changed as of GoLand 2023.3 via GO-5841 🎉Technically adding to
Reporter
is a breaking change but as covered in this comment:Either way I think it's better to land these ASAP to reduce the blast radius then to carry them around for possibly a lot longer - note that I'm not strictly against deprecating/removing
PrintText
andPrintError
though I don't think there's a lot of value in keeping them.As penance, I've also added rich method comments for the interface.
(having said that, since this is a breaking change already maybe we should just remove
PrintText
andPrintError
right now)