-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Use Option pattern for componenthelper #2778
Conversation
4588a1b
to
e547e2f
Compare
Codecov Report
@@ Coverage Diff @@
## main #2778 +/- ##
=======================================
Coverage 91.71% 91.71%
=======================================
Files 293 293
Lines 15609 15614 +5
=======================================
+ Hits 14316 14321 +5
+ Misses 887 886 -1
- Partials 406 407 +1
Continue to review full report at Codecov.
|
d952987
to
9761974
Compare
exporter/exporterhelper/common.go
Outdated
@@ -25,7 +25,7 @@ import ( | |||
"go.opentelemetry.io/collector/config/configmodels" | |||
) | |||
|
|||
// ComponentSettings for timeout. The timeout applies to individual attempts to send data to the backend. | |||
// Settings for timeout. The timeout applies to individual attempts to send data to the backend. |
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.
go lint
has yelled at me enough times I've internalized that godoc should start with TimeoutSettings
, though not sure how to reword this doing so and it seems that this already was deviating from that guidance. v0v
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.
Not sure what we missed in the config that this is not picked by lint. Will look into this
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.
Also was done unintentionally by the IDE because I renamed ComponentSettings -> Settings initially and text was also replaced
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.
Apparently it was disabled, starting to enable bunch of these #2786 will take a bit to fix all errors.
Signed-off-by: Bogdan Drutu <[email protected]>
dd8105f
to
dd92465
Compare
Has a merge conflict. |
Signed-off-by: Bogdan Drutu [email protected]