search

opentracing / opentracing-php

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

Formats and Tags constants #103

Closed piotrooo closed 4 years ago

piotrooo commented 4 years ago

What do you think about encapsulate a constants from Formats and Tags into class?

Why?

After that change we can change composer.json autoloading, to be PSR-4-fully compatible - now we include some files manually. Further, we will be able to recognize context of used constant. It's difference when you try to read Formats::TEXT_MAP instead of TEXT_MAP, you don't have to thinking in what context is TEXT_MAP used.

jcchavezs commented 4 years ago

+1 to this. I guess these are the things I learnt in the way. Thanks for pointing it out.

We use constants in zipkin-php: https://github.com/openzipkin/zipkin-php/blob/master/src/Zipkin/Kind.php.

Are you up to coming with a PR?

jcchavezs commented 4 years ago

I am so sorry I misunderstood the issue 🤦‍♂ . I thought you suggested the other way around, in any case I have some questions (cc @cawolf):

  1. I agree having PSR4 is a benefit, I could not find any reference to PS4 saying we should move away from namespace constants (nor a way to support them). Do you have any link talking about this?
  2. I don't fully agree with the context argument as they are namespaced hence A::B is as descriptive as A\B
cawolf commented 4 years ago
  1. I agree having PSR4 is a benefit, I could not find any reference to PS4 saying we should move away from namespace constants (nor a way to support them). Do you have any link talking about this?

Yes, I would be interested too. To be honest, I did not encounter namespace constants in the wild before, and some code quality tools have problems identifying them. However, as a user, I don't care if the parent of the constant is a namespace or a class.

  1. I don't fully agree with the context argument as they are namespaced hence A::B is as descriptive as A\B

I would second that, OpenTracing\Tags\SPAN_KIND and OpenTracing\Tags::SPAN_KIND are at the same cognitive level.

piotrooo commented 4 years ago

As @cawolf said, I also didn't encounter constant defined in that way.

Constant should be bundled with the class, using constants without class (thanks for PHP for allow this...) looks like code bad smells, no other language allows to do that.

For defining const in that way, should be used define function.

For me this OpenTracing\Tags\SPAN_KIND and OpenTracing\Tags::SPAN_KIND makes difference. And I prefer second option.

cawolf commented 4 years ago

One thing to consider: is changing OpenTracing\Tags\SPAN_KIND to OpenTracing\Tags::SPAN_KIND considered a breaking change under semver? If so, I would strongly argue against changing this, because it would imply releasing 2.0 before we even had a proper 1.0.

At least for client implementations upgrading from 1.0.0@beta5 to 1.0.0 would mean changing their code.

jcchavezs commented 4 years ago

So after some research and chats on this I learnt PSR4 is the standard for autoloading but manually loading files is not discouraged, it is just a different use case so being "PSR4 compliant" does not mean we should drop the file loading.

The "context" claim is more like a personal preference IMHO.

Just a final mention. When I started this library I created these constants as class constants but then made them actual constants. See https://github.com/jcchavezs/opentracing-php/issues/2.

cawolf commented 4 years ago

And I think that decision should not be reverted right now. To reiterate:

as a user, I don't care if the parent of the constant is a namespace or a class

and

OpenTracing\Tags\SPAN_KIND and OpenTracing\Tags::SPAN_KIND are at the same cognitive level

So I would vote for closing this issue without changes to the code.