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

Investigate if we can/want to make fields and return values @Nonnull by default. #2337

Open
Oberon00 opened this issue Dec 17, 2020 · 1 comment
Labels

Comments

@Oberon00
Copy link
Member

https://stackoverflow.com/a/11807961/2128694 suggests there is a way to annotate a package so that fields and/or return values are @Nonnull by default, similarly to the @ParametersAreNonnullByDefault we already use. This answer is for findbugs, but may also work with our errorprone.

Relatedly, we may want to improve checking for these annotations: #1768

@Oberon00 Oberon00 added the Feature Request Suggest an idea for this project label Dec 17, 2020
@edysli
Copy link

edysli commented Oct 15, 2022

Would a custom annotation like Spring's @NonNullApi work for this project?

I'd be happy to submit a PR to add it. :) Just let me know in which subproject/package it would fit.

edysli added a commit to edysli/opentelemetry-java-instrumentation that referenced this issue Oct 15, 2022
Refactor class `ContainerResource` so that its methods return
`Optional`s instead of `null`. This makes for safer code that doesn't
rely on `null` to signal the absence of result. Moreover,
`Optional.map()` and `Optional.orElseGet()` use the same functional
style as `Stream`, which is well readable.

Issue-Id: open-telemetry#6694
Issue-Id: open-telemetry/opentelemetry-java#2337
edysli added a commit to edysli/opentelemetry-java-instrumentation that referenced this issue Oct 15, 2022
Several test cases in `ContainerResourceTest` have the same outcome,
so instead of duplicating the assertions inside each test method, take
advantage of `@ParameterizedTest` to inject inputs and expected
outputs multiple times into the same test. This improves test code
readability by reducing test method length and making identical cases
more obvious.

Also rename test methods to better express the tested code's expected
behaviour.

Issue-Id: open-telemetry#6694
Issue-Id: open-telemetry/opentelemetry-java#2337
edysli added a commit to edysli/opentelemetry-java-instrumentation that referenced this issue Oct 15, 2022
Refactor class `ContainerResource` so that its methods return
`Optional`s instead of `null`. This makes for safer code that doesn't
rely on `null` to signal the absence of result. Moreover,
`Optional.map()` and `Optional.orElseGet()` use the same functional
style as `Stream`, which is well readable.

Issue-Id: open-telemetry#6694
Issue-Id: open-telemetry/opentelemetry-java#2337
edysli added a commit to edysli/opentelemetry-java-instrumentation that referenced this issue Oct 15, 2022
Several test cases in `ContainerResourceTest` have the same outcome,
so instead of duplicating the assertions inside each test method, take
advantage of `@ParameterizedTest` to inject inputs and expected
outputs multiple times into the same test. This improves test code
readability by reducing test method length and making identical cases
more obvious.

Also rename test methods to better express the tested code's expected
behaviour.

Issue-Id: open-telemetry#6694
Issue-Id: open-telemetry/opentelemetry-java#2337
edysli added a commit to edysli/opentelemetry-java-instrumentation that referenced this issue Oct 17, 2022
Several test cases in `ContainerResourceTest` have the same outcome,
so instead of duplicating the assertions inside each test method, take
advantage of `@ParameterizedTest` to inject inputs and expected
outputs multiple times into the same test. This improves test code
readability by reducing test method length and making identical cases
more obvious.

Also rename test methods to better express the tested code's expected
behaviour.

Issue-Id: open-telemetry#6694
Issue-Id: open-telemetry/opentelemetry-java#2337
trask added a commit to open-telemetry/opentelemetry-java-instrumentation that referenced this issue Oct 19, 2022
…e` to avoid using null (#6889)

While I was looking at issues
#6694 and
open-telemetry/opentelemetry-java#2337, I saw that the code in
`io.opentelemetry.instrumentation.resources.ContainerResource` used
`null` several times as return value which isn't safe. Nowadays,
`Optional` is better suited to signal the absence of a result, so I
refactored `ContainerResource` to use `Optional`s instead of null.

On the way, I also refactored this class's unit tests into parameterised
tests to reduce test code duplication. These improvements should help
implementing a solution to
#6694.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
dmarkwat pushed a commit to dmarkwat/opentelemetry-java-instrumentation that referenced this issue Oct 22, 2022
…e` to avoid using null (open-telemetry#6889)

While I was looking at issues
open-telemetry#6694 and
open-telemetry/opentelemetry-java#2337, I saw that the code in
`io.opentelemetry.instrumentation.resources.ContainerResource` used
`null` several times as return value which isn't safe. Nowadays,
`Optional` is better suited to signal the absence of a result, so I
refactored `ContainerResource` to use `Optional`s instead of null.

On the way, I also refactored this class's unit tests into parameterised
tests to reduce test code duplication. These improvements should help
implementing a solution to
open-telemetry#6694.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this issue Oct 23, 2022
…e` to avoid using null (open-telemetry#6889)

While I was looking at issues
open-telemetry#6694 and
open-telemetry/opentelemetry-java#2337, I saw that the code in
`io.opentelemetry.instrumentation.resources.ContainerResource` used
`null` several times as return value which isn't safe. Nowadays,
`Optional` is better suited to signal the absence of a result, so I
refactored `ContainerResource` to use `Optional`s instead of null.

On the way, I also refactored this class's unit tests into parameterised
tests to reduce test code duplication. These improvements should help
implementing a solution to
open-telemetry#6694.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this issue Oct 31, 2022
…e` to avoid using null (open-telemetry#6889)

While I was looking at issues
open-telemetry#6694 and
open-telemetry/opentelemetry-java#2337, I saw that the code in
`io.opentelemetry.instrumentation.resources.ContainerResource` used
`null` several times as return value which isn't safe. Nowadays,
`Optional` is better suited to signal the absence of a result, so I
refactored `ContainerResource` to use `Optional`s instead of null.

On the way, I also refactored this class's unit tests into parameterised
tests to reduce test code duplication. These improvements should help
implementing a solution to
open-telemetry#6694.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this issue Dec 4, 2022
…e` to avoid using null (open-telemetry#6889)

While I was looking at issues
open-telemetry#6694 and
open-telemetry/opentelemetry-java#2337, I saw that the code in
`io.opentelemetry.instrumentation.resources.ContainerResource` used
`null` several times as return value which isn't safe. Nowadays,
`Optional` is better suited to signal the absence of a result, so I
refactored `ContainerResource` to use `Optional`s instead of null.

On the way, I also refactored this class's unit tests into parameterised
tests to reduce test code duplication. These improvements should help
implementing a solution to
open-telemetry#6694.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants