Add nullable annotation to Tracer#extract

[realark]

• Add findbugs annotations as optional dependency (chosen due to widespread IDE support and java6 support)
• Add @Nullable annotation to Tracer#extract

Closes https://github.com/opentracing/opentracing-java/issues/168

Closes https://github.com/opentracing/opentracing-java/pull/203

[coveralls]

Coverage increased (+0.7%) to 74.112% when pulling caf76e28758916cdb7e9a8dd2b848df7036d1d5a on realark:ark/nullable-annotations into cb40981e725125791afa4cc496eeb21424108c50 on opentracing:master.

[carlosalberto]

Maybe @yurishkuro @pavolloffay have something to comment here? ;)

[pavolloffay]

I think the IDE would show you the help message only if the scope f the dependency were compile.

I don't think it's worth to add an extra dependency to API artifact for @Nullable specially if there are very little use cases when null is returned.

[sjoerdtalsma]

I don't think it's worth to add an extra dependency to API...

Especially since JSR 305 exposes a 'split package' (javax.annotation being used in JEE libraries as well) that does not play well with the java 9 module system (project jigsaw)!

[mabn]

@pavolloffay it does not add a dependency to the api artifact, See the description in https://github.com/opentracing/opentracing-java/pull/203 (which probably could be closed now)

[sjoerdtalsma]

@mabn

it does not add a dependency to the api artifact

How can this not be considered adding a dependency when the annotation is actually used in the API code signatures?

    @Nullable
    <C> SpanContext extract(Format<C> format, C carrier);

This compiles ok, but will fail in my project if I don't have javax.annotation.Nullable on the classpath. Worse yet, as I mentioned above the package javax.annotation is a split package declared by different libraries and JEE itself, which causes problems in Java 9 modularized code.

I also think it is not worth the trouble (at least until the whole split package is resolved by Oracle blessing one big module that contains all current javax.annotation classes).

[mabn]

but will fail in my project if I don't have javax.annotation.Nullable on the classpath

As far as I know - it wont, the annotation jar/class is not needed at runtime. Guava had jar305 as an optional dependency for a long time (they changed it to runtime only in ver 22 for an unrelated reason)

[sjoerdtalsma]

@mabn Wouldn't reflecting the extract method attempt to return the @Nullable annotation?

[felixbarny]

Annotations don't need to be present at runtime. When reflectively accessing a methods annotations, only those which are actually on the classpath are returned. No ClassNotFoundExceptions are thrown.

[sjoerdtalsma]

@felixbarny Thanks, learned something again :smile:

Annotations don't need to be present at runtime. When reflectively accessing a methods annotations, only those which are actually on the classpath are returned. No ClassNotFoundExceptions are thrown.

Good to know! That takes care of my main concern.

[pavolloffay]

@felixbarny @realark does IDE show warning message even though the scope of jsr305 is optional?

[felixbarny]

IntelliJ 2018.1 does take the annotations into account even when they are not on the classpath. I'd assume the AST IDEs create are based on the bytecode and that they don't use reflection to introspect classes.

I would however be consistent in declaring the annotations to either be <scope>provided</scope> or <optional>true</optional>. I'm also not quite sure what the better approach is. I have used the provided scope.

[realark]

@pavolloffay Intellij 2017.3.4 does show the warning even with the optional scope.

@felixbarny (or anyone else) any objections to using <optional>true</optional> in both declarations?

[felixbarny]

Is <optional>true</optional> even required to be set in <dependencyManagement>? It's basically just to avoid having to specify the version in the child project, right? I'd also add a small note that annotations are not required to be available at runtime, as this is something not everyone knows.

[sjoerdtalsma]

I would prefer to leave the dependency out of <dependencyManagement> entirely, as it is optional anyway.

@felixbarny

I would however be consistent in declaring the annotations to either be <scope>provided</scope> or <optional>true</optional>. I'm also not quite sure what the better approach is. I have used the provided scope.

What was the reasoning of not having it both optional and provided? :wink:

[felixbarny]

What was the reasoning of not having it both optional and provided? 😉

That probably works as well. When I wrote that, I didn't realize that one declaration was inside <dependencyManagement> so I thought it was declared optional in one place and provided in another one.