feat(api-logs)!: Marked private methods as "conventionally private". #5789
Conversation
…/opentelemetry-js into private-methods-api-5734
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5789 +/- ##
=======================================
Coverage 95.01% 95.01%
=======================================
Files 303 303
Lines 7946 7946
Branches 1607 1607
=======================================
Hits 7550 7550
Misses 396 396
🚀 New features to boost your workflow:
|
| new ProxyLogger(this, name, version, options) | ||
| ); | ||
| } | ||
|
|
||
| getDelegate(): LoggerProvider { |
There was a problem hiding this comment.
Looks like this one was added for testing purposes only, why removing the tests?
There was a problem hiding this comment.
Oh I didn't know that we add methods for testing purposes only. I can make it private then and add it back to the tests.
There was a problem hiding this comment.
I'm not sure about the original purpose, it may be that this logic was duplicated from ProxyTracerProvider, but I can see is only used in the project for testing :)
There was a problem hiding this comment.
I put this method back and added a note that it's used for tests only. And added an @internal annotation to it.
| /** | ||
| * Set the delegate logger provider | ||
| */ | ||
| setDelegate(delegate: LoggerProvider) { |
There was a problem hiding this comment.
Why not making these as private as well?
There was a problem hiding this comment.
I can see the code is being used outside, and not really easy to refactor the code to support it, then the question is about package-private @pichlermarc mentioned in the bug, is it sufficient to add the underscore in the method names? this will not prevent people to use it
There was a problem hiding this comment.
I know that what it means to have them as "conventially private" but not sure if this really useful, if we take this approach, we need to add typedoc @internal annotations to avoid having these methods exposed in docs
https://open-telemetry.github.io/opentelemetry-js/classes/_opentelemetry_api-logs.ProxyLoggerProvider.html
There was a problem hiding this comment.
I went with what the issue recommended we do but we can discuss alternatives if you like. I added the @internal annotation in the meantime
There was a problem hiding this comment.
I think adding @internal is good enough for this here. The goal is to actually remove this whole class from exports eventually, since our end users are not expected to use it directly. This means that the types won't be available, see #5733.
…/opentelemetry-js into private-methods-api-5734
| * Used by tests only. | ||
| * @internal | ||
| */ | ||
| _getDelegate(): LoggerProvider { |
There was a problem hiding this comment.
you can also make it private and it will not be exposed in intellisense, but you can still access it with provider['_getDelegate'](). Not sure if we need to go that far though? @open-telemetry/javascript-maintainers what do you think?
There was a problem hiding this comment.
@dyladan Should I make this change or wait for a reply back from others?
There was a problem hiding this comment.
I think this would be addressed by #5733, if we don't export it it does not necessarily have to be private. (so good to leave it as-is for now) :)
| /** | ||
| * Set the delegate logger provider | ||
| */ | ||
| setDelegate(delegate: LoggerProvider) { |
There was a problem hiding this comment.
I think adding @internal is good enough for this here. The goal is to actually remove this whole class from exports eventually, since our end users are not expected to use it directly. This means that the types won't be available, see #5733.
| * Used by tests only. | ||
| * @internal | ||
| */ | ||
| _getDelegate(): LoggerProvider { |
There was a problem hiding this comment.
I think this would be addressed by #5733, if we don't export it it does not necessarily have to be private. (so good to leave it as-is for now) :)
Which problem is this PR solving?
Reviewed properties and methods that are suppose to be private and changed the delegate methods to private.
Fixes #5734
Short description of the changes
getDelegatesince it's not being usedsetDelegateto be privategetDelegateLoggerto be privateType of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: