Skip to content
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

Deprecate funcs that repeate receiver in name #11287

Merged

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Sep 27, 2024

This change makes the names to be more "Go" friendly. Also it will not become an issue that types will try to implement different Factory types (receiver,processor) because that is forbidden because of our design. This also makes the names consistent with connector.Factory.

@bogdandrutu bogdandrutu requested a review from a team as a code owner September 27, 2024 16:40
@bogdandrutu bogdandrutu force-pushed the deprecate-receiver-name branch 3 times, most recently from 2d216d1 to e634db1 Compare September 27, 2024 16:49
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 62.71186% with 22 lines in your changes missing coverage. Please review.

Project coverage is 91.45%. Comparing base (431fd11) to head (cf06b16).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
receiver/receiver.go 55.55% 12 Missing ⚠️
receiver/receiverprofiles/profiles.go 33.33% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11287      +/-   ##
==========================================
- Coverage   91.52%   91.45%   -0.08%     
==========================================
  Files         424      424              
  Lines       20222    20238      +16     
==========================================
  Hits        18509    18509              
- Misses       1329     1345      +16     
  Partials      384      384              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Since we're reconsidering the names, I'm curious if Create is meaningfully different than New or if e.g. factory.NewLogs would be more idiomatic.

@bogdandrutu
Copy link
Member Author

Since we're reconsidering the names, I'm curious if Create is meaningfully different than New or if e.g. factory.NewLogs would be more idiomatic.

That would mean I have to change connectors as well :(((... Happy to do it if others feel the same.

@dmitryax
Copy link
Member

New may be a better name for functions, but:

  1. We would also need to rename all the CreateLogsFunc functions to NewLogsFunc, which is not that appealing to me...
  2. AFAIK New is typically used as a standalone function, not as a factory method. This is what we've been doing as well.

So I would lean towards keeping Create.

@bogdandrutu bogdandrutu merged commit bce1040 into open-telemetry:main Sep 30, 2024
48 of 49 checks passed
@bogdandrutu bogdandrutu deleted the deprecate-receiver-name branch September 30, 2024 16:19
@github-actions github-actions bot added this to the next release milestone Sep 30, 2024
bogdandrutu added a commit that referenced this pull request Sep 30, 2024
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this pull request Oct 8, 2024
This change makes the names to be more "Go" friendly. Also it will not
become an issue that types will try to implement different Factory types
(receiver,processor) because that is forbidden because of our design.
This also makes the names consistent with connector.Factory.

Signed-off-by: Bogdan Drutu <[email protected]>
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this pull request Oct 8, 2024
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.

4 participants