opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
508 stars 56 forks source link

Should we include default value for `ScopeManager::activate(OpenTracingSpan $span, $finishSpanOnClose)` #63

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

Background

At the moment there is default value for $finishSpanOnClose and since StartSpanOptions includes a default value for this: https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/StartSpanOptions.php#L30

Problem

One problem is that it is uncomfortable to have to pass the value all the time but also there is an inconsistency problem because https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/StartSpanOptions.php#L30 has a default value whereas https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/ScopeManager.php#L21 does not.

Proposal

Either remove default value on both or either pass a default value for both.

Ping @yurishkuro @ellisv @beberlei @tedsuo

yurishkuro commented 6 years ago

my vote is to pass the default value in both cases

jcchavezs commented 6 years ago

@yurishkuro as true or false by default?

yurishkuro commented 6 years ago

In thought it was already settled that true is more appropriate for PHP.

tedsuo commented 6 years ago

@jcchavezs it should be true by default. I think it's fine to set a default in both cases.

We should add language to the Scopes RFC to indicate that true is the only valid default, it's a little vague on this.

jcchavezs commented 6 years ago

Great @tedsuo. I will open a RFC.