-
Notifications
You must be signed in to change notification settings - Fork 806
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
Conversation
4b2554b
to
76d6309
Compare
I think it would be better if we merge and release the API changes before reviewing this here. cc: @open-telemetry/javascript-maintainers |
This is a draft I opened when I initially implemented bind/with changes, @dyladan. Do you want to merge those as well? Close this PR if not. |
Codecov Report
@@ 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
|
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. |
I see this is still a draft. Are you still working on this? |
Sorry. I've been on a holiday for a week. Let me take a quick check on Monday. |
fd87766
to
c43df2c
Compare
c43df2c
to
55c6f71
Compare
I'm a bit lost with the error for node v16. |
.github/workflows/unit-test.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it!
Companion PR to open-telemetry/opentelemetry-js-api#78 for SDK changes.