"Applying" potential Copilot code review rules#2182
Conversation
github-actions
Bot
assigned
atoulme,
breedx-splk,
HaloFour,
jack-berg,
jackshirazi,
jkwatson,
JonasKunz,
jsuereth,
KarstenSchnitter and
LikeTheSalad
trask
changed the title
KarstenSchnitter
left a comment
KarstenSchnitter
left a comment
There was a problem hiding this comment.
I reviewed the changes to the cloudfoundry-resources subproject. They look fine, I approve.
SylvainJuge
left a comment
SylvainJuge
left a comment
There was a problem hiding this comment.
LGTM for the following modules:
inferred-spansjmx-metricsjmx-scraperspan-stacktrace
ce7b79f to
2ae0ed8
Compare
| 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.
Seems harmless but I don't immediately see why it's required.
There was a problem hiding this comment.
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.
oh wild! thanks for seeing that through...did not expect that. :)
| Arrays.asList( | ||
| asList( |
There was a problem hiding this comment.
Hmmm, this is fine I guess, but this one seems less idiomatic to me.
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
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
breedx-splk
left a comment
There was a problem hiding this comment.
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) |
13 participants
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)