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

Enable PrivateConstructorForUtilityClass errorprone check #6427

Merged
merged 18 commits into from
Aug 10, 2022

Conversation

trask
Copy link
Member

@trask trask commented Aug 6, 2022

Primarily motivated by having (mostly) same errorprone configuration/checks as the other Java repos.

@trask trask marked this pull request as ready for review August 6, 2022 05:09
@trask trask requested a review from a team as a code owner August 6, 2022 05:09
@trask trask marked this pull request as draft August 6, 2022 05:59
@trask trask marked this pull request as ready for review August 6, 2022 21:47
Comment on lines 39 to 40
@SuppressWarnings({"PrivateConstructorForUtilityClass", "unused"})
public static class DemoServlet3Advice {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could do

if (name.endsWith("Advice")) {
  disable("PrivateConstructorForUtilityClass")
}

instead of suppressing this in every advice class? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately this doesn't work (the gradle block is only resolved once, not per class name)

but this suppression on all of the advice classes is sufficiently horrendous, so I went ahead and added a custom OtelPrivateConstructorForUtilityClass check that ignores Advice classes

@@ -54,4 +54,6 @@ static boolean isLocalRoot(Context parentContext) {
static Context store(Context context, Span span) {
return context.with(KEY, span);
}

private LocalRootSpan() {}
Copy link
Member

Choose a reason for hiding this comment

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

Oops 🙈

@laurit laurit merged commit 68a9f20 into open-telemetry:main Aug 10, 2022
@trask trask deleted the errorprone branch August 10, 2022 15:51
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
…etry#6427)

* PrivateConstructorForUtilityClass

* Advice

* More advice

* More

* More advice

* More

* Spotless

* Fix

* Fix

* Fix

* A better solution

* Revert

* More

* Fix

* Spotless

* Fix
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
…etry#6427)

* PrivateConstructorForUtilityClass

* Advice

* More advice

* More

* More advice

* More

* Spotless

* Fix

* Fix

* Fix

* A better solution

* Revert

* More

* Fix

* Spotless

* Fix
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
…etry#6427)

* PrivateConstructorForUtilityClass

* Advice

* More advice

* More

* More advice

* More

* Spotless

* Fix

* Fix

* Fix

* A better solution

* Revert

* More

* Fix

* Spotless

* Fix
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.

None yet

3 participants