-
Notifications
You must be signed in to change notification settings - Fork 169
"Applying" potential Copilot code review rules #2182
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
Conversation
KarstenSchnitter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the changes to the cloudfoundry-resources subproject. They look fine, I approve.
SylvainJuge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the following modules:
inferred-spansjmx-metricsjmx-scraperspan-stacktrace
ce7b79f to
2ae0ed8
Compare
aws-resources/build.gradle.kts
Outdated
| testImplementation("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure") | ||
| testImplementation("io.opentelemetry:opentelemetry-api-incubator") | ||
| testImplementation("io.opentelemetry:opentelemetry-sdk-testing") | ||
| testImplementation("com.google.auto.service:auto-service") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems harmless but I don't immediately see why it's required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out this is required, because javac -Werror on the test files throws an error if it can't find the annotations used on the classes that the tests reference 🤯
Which I assume affects downstream consumer of our libraries also as long as we make autoservice a compileOnly dependency.
so I reverted all the autoservice changes in this PR so we can discuss it separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wild! thanks for seeing that through...did not expect that. :)
...ain/java/io/opentelemetry/contrib/awsxray/propagator/internal/AwsConfigurablePropagator.java
Outdated
Show resolved
Hide resolved
...s/src/main/java/io/opentelemetry/contrib/azure/resource/AzureAppServiceResourceProvider.java
Outdated
Show resolved
Hide resolved
...ng/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/NoopExporterCallback.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/SignalStorageExporter.java
Outdated
Show resolved
Hide resolved
| Arrays.asList( | ||
| asList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, this is fine I guess, but this one seems less idiomatic to me.
processors/src/main/java/io/opentelemetry/contrib/filter/FilteringLogRecordProcessor.java
Outdated
Show resolved
Hide resolved
breedx-splk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @trask and I talked a little bit about this out of band. I think it's good, but we agreed that the classes in "internal" packages (and also package-private) should retain their final designations (which this PR attempts to remove).
The final is good because it also discourages other developers, even within the project itself, from introducing inheritance, and it maintains the status quo.
Otherwise, looks pretty good.
3049362 to
3807cfd
Compare
2c76010 to
5d3375f
Compare
5d3375f to
e514e99
Compare
|
looks like improving |
I'll try that in a follow-up (didn't try it yet) |
|
Was a11d406 just done with an abundance of caution because users might have done inheritance? I wish we could keep that, but won't hold this PR up with it. |
breedx-splk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's in pretty good shape now.
yeah, I plan to send that in a follow-up, realized I should draw a bit more attention to that change since it's a breaking change (to alpha modules) |
Hopefully this is reflective of the changes copilot code review will suggest after #2170.
Please comment either here or there if you have suggestions about the
style-guide.mdthat was used to generate this PR.This PR was generated by running this prompt module-by-module:
I made very minor manual fixes to Copilot's work after the fact, including reverting adding
finalto public classes in the stableaws-xraymodule.(I skipped the
consistent-samplingmodule for now, will apply to that later)