Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

chore: do not export singletons #46

Merged
merged 7 commits into from
May 27, 2021

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Apr 23, 2021

Fixes #38

@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #46 (40e151f) into main (4cda9bd) will decrease coverage by 0.04%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   94.70%   94.66%   -0.05%     
==========================================
  Files          42       42              
  Lines         567      562       -5     
  Branches       94       94              
==========================================
- Hits          537      532       -5     
  Misses         30       30              
Impacted Files Coverage Δ
src/index.ts 100.00% <ø> (ø)
src/propagation/NoopTextMapPropagator.ts 71.42% <ø> (-3.58%) ⬇️
src/trace/NoopTracer.ts 95.65% <ø> (-0.10%) ⬇️
src/trace/NoopTracerProvider.ts 80.00% <50.00%> (-3.34%) ⬇️
src/api/propagation.ts 100.00% <100.00%> (ø)
src/trace/ProxyTracer.ts 100.00% <100.00%> (ø)
src/trace/ProxyTracerProvider.ts 93.33% <100.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cda9bd...40e151f. Read the comment docs.

src/api/context.ts Outdated Show resolved Hide resolved
src/trace/NoopTracer.ts Show resolved Hide resolved
@Flarna
Copy link
Member

Flarna commented Apr 29, 2021

I know that the headline tells do not export singletons. But above I read ... does not require that we export any no-op classes or singletons.

Currently NoopTextMapPropagator, NoopTracer,.. are still exported. Shoudl we remove them also?

Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

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

I guess the followup PR in core will be funny :o).

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

it looks fine, but before this one is merged, we should first update core and contrib to avoid problems with being unable to make a release of those packages for a long time. Merging this PR should be a last thing in this task

@dyladan dyladan linked an issue May 5, 2021 that may be closed by this pull request
@Flarna Flarna mentioned this pull request May 17, 2021
@dyladan dyladan merged commit 950433f into open-telemetry:main May 27, 2021
@dyladan dyladan deleted the no-leak-singletons branch May 27, 2021 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider not exposing type + singleton
4 participants