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

feat: unify the signatures of bind and with #2247

Merged
merged 10 commits into from
Jun 30, 2021

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Jun 2, 2021

Companion PR to open-telemetry/opentelemetry-js-api#78 for SDK changes.

@rauno56 rauno56 marked this pull request as draft June 2, 2021 16:32
@rauno56
Copy link
Member Author

rauno56 commented Jun 2, 2021

I think it would be better if we merge and release the API changes before reviewing this here.
Current changes should be sufficient, but the CI doesn't know that because I don't have a compatible API version to reference here.

cc: @open-telemetry/javascript-maintainers

@rauno56
Copy link
Member Author

rauno56 commented Jun 11, 2021

This is a draft I opened when I initially implemented bind/with changes, @dyladan.
You got ahead of me by merging most of the same changes, synced the branch and these are what was left from mine - some internal API changes.

Do you want to merge those as well? Close this PR if not.

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #2247 (4616497) into main (0141897) will increase coverage by 0.31%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main    #2247      +/-   ##
==========================================
+ Coverage   92.47%   92.79%   +0.31%     
==========================================
  Files         122      145      +23     
  Lines        4131     5216    +1085     
  Branches      858     1068     +210     
==========================================
+ Hits         3820     4840    +1020     
- Misses        311      376      +65     
Impacted Files Coverage Δ
...kages/opentelemetry-web/src/StackContextManager.ts 94.28% <66.66%> (ø)
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 97.01% <100.00%> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <100.00%> (ø)
...kages/opentelemetry-api-metrics/src/api/metrics.ts 90.90% <0.00%> (ø)
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <0.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.71% <0.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 97.58% <0.00%> (ø)
packages/opentelemetry-web/src/utils.ts 94.80% <0.00%> (ø)
...kages/opentelemetry-exporter-collector/src/util.ts 100.00% <0.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <0.00%> (ø)
... and 17 more

@dyladan
Copy link
Member

dyladan commented Jun 16, 2021

Yes I think it should be unified and since this is internal methods it should be no problem. Please mark this as ready for review if it is ready.

@dyladan
Copy link
Member

dyladan commented Jun 23, 2021

I see this is still a draft. Are you still working on this?

@rauno56
Copy link
Member Author

rauno56 commented Jun 25, 2021

Sorry. I've been on a holiday for a week. Let me take a quick check on Monday.

@rauno56 rauno56 marked this pull request as ready for review June 28, 2021 13:19
@rauno56 rauno56 requested a review from MSNev as a code owner June 28, 2021 13:19
@rauno56
Copy link
Member Author

rauno56 commented Jun 28, 2021

I'm a bit lost with the error for node v16.

@@ -46,6 +46,7 @@ jobs:
- name: Install and Build (cache hit) 🔧
if: steps.cache.outputs.cache-hit == 'true'
run: |
chown -R 1001:121 "/github/home/.npm" || true # fix npm cache permissions for npm v7 if cache exists
Copy link
Member

Choose a reason for hiding this comment

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

Node 16 error was fixed in another PR please remove this

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants