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

Address issues #13 and #41 #45

Merged
merged 4 commits into from
Jan 11, 2019
Merged

Conversation

mwear
Copy link
Member

@mwear mwear commented Jan 10, 2019

#41 has been open for comment for quite some time, and all feedback has been in favor of the suggested changes. While discussing #41 there was interest in addressing #13 to provide symmetry between the Tracer#start_span and Tracer#start_active_span APIs. This PR addresses both #13 and #41.

Tracer#start_span now accepts an optional block. When a block is given, it returns the block's return value, otherwise it returns the newly-created span. See #13.

When Tracer#start_active_span is given an optional block, it will now return the block's return value, otherwise it returns the newly-created scope. This is a change from the previous behavior where it always returned the newly-created scope. See #41.

This commit addresses the suggestion from issue opentracing#41. If
Tracer#start_active_span is given an optional block, it will return
the block's return value, otherwise it will return the newly-created
scope.
This commit addresses the suggestion in issue opentracing#13 and now allows
Tracer#start_span to accept a block. When the block is given, it
returns the block's return value, otherwise it returns
the newly-created span.
@ethanjcohen-underarmour

🎉 Whoo! LGTM. We work closely with LightStep so once this gets published as a gem version I'll suggest they update their ruby gem as well.

Copy link
Contributor

@indrekj indrekj left a comment

Choose a reason for hiding this comment

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

👍

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.

3 participants