-
Notifications
You must be signed in to change notification settings - Fork 145
Conversation
1cd73d8
to
b5de1b7
Compare
3 similar comments
lgtm @bcronin ok to merge? |
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.
Looks good.
With regards to, "functionalities that are readily available in pure JS", I'm not sure how old of platforms opentracing-javascript
is supposed to support. Given that this affects the mock tracer only, I'm not very worried about this. However, if it does seem concerning, it also seems like it'd be very easy to rewrite these as pure for
loops.
This looks 👍 as-is to me, though given that it's the mock tracer only.
For future reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys#Browser_compatibility Both features are also supported by all Node versions |
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.
Thank you very much @rochdev ! Should we merge? Also do we plan to ship a new release or do we want to include other fixes before?
I think it's fine to do a new release |
Great @yurishkuro ! who's in charge for that? Something I can help with? |
I can probably do the release, but could use some help with preparing a PR with changelog changes |
That would be great! sure thing we'll help with the changelog (so me or @rochdev ) |
I can help with the changelog. I'll try to have a PR later on today. |
See #106 |
Published 0.14.3 |
This PR removes the dependency on
lodash
since it was causing dependency problems for depending libraries. It was also only used for functionalities that are readily available in pure JS. This problem existed as well whenunderscore
was used instead oflodash
(#75).Fixes #95