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

Remove ScopeBound... example implementations #819

Merged

Conversation

Nevay
Copy link
Contributor

@Nevay Nevay commented Sep 9, 2022

Removes ScopeBound... classes to reduce the API surface.

  • ScopeBoundCallable: Was intended for event loops. Event loop decorators should instead implement this in a way that fits the specific loop API the best with regard to avoiding unnecessary references to arguments and error handling.
    (The implementation was (mostly) sufficient for react/event-loop but shouldn't be used for revolt/event-loop.)
  • ScopeBoundPromise: Was only intended as very basic example implementation. An actual implementation has to hook into the specific promise implementation to capture the context of the initial promise.
    (I'm not sure how useful promise/future support is in general, there are scenarios where we cannot preserve the context association, e.g. when extracting/awaiting the value.)

@Nevay Nevay requested a review from a team as a code owner September 9, 2022 14:21
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #819 (bf38311) into main (6e385b9) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head bf38311 differs from pull request most recent head aab5c05. Consider uploading reports for the commit aab5c05 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #819      +/-   ##
============================================
- Coverage     82.88%   82.80%   -0.08%     
+ Complexity     1829     1803      -26     
============================================
  Files           225      222       -3     
  Lines          4697     4653      -44     
============================================
- Hits           3893     3853      -40     
+ Misses          804      800       -4     
Flag Coverage Δ
7.4 ?
8.0 ?
8.1 82.80% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SDK/Metrics/Meter.php 99.08% <0.00%> (-0.92%) ⬇️
src/SDK/Common/Dsn/Factory.php 100.00% <0.00%> (ø)
src/SDK/Common/Util/WeakMap.php 0.00% <0.00%> (ø)
src/Context/ContextStorageNode.php 100.00% <0.00%> (ø)
src/Context/FiberBoundContextStorageScope.php 0.00% <0.00%> (ø)
src/Contrib/Context/Swoole/SwooleContextScope.php 0.00% <0.00%> (ø)
src/SDK/Common/Attribute/AttributesBuilder.php 84.84% <0.00%> (+1.51%) ⬆️

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 6e385b9...aab5c05. Read the comment docs.

Nevay added a commit to Nevay/opentelemetry-php that referenced this pull request Sep 9, 2022
Drop if open-telemetry#819 is merged first.
@brettmc brettmc merged commit 776273c into open-telemetry:main Sep 10, 2022
@Nevay Nevay deleted the remove-scopebound-examples-classes branch April 19, 2023 19:11
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.

None yet

2 participants