-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[chore] apply fieldalignment linter #9133
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9133 +/- ##
=======================================
Coverage 91.47% 91.47%
=======================================
Files 320 320
Lines 17187 17187
=======================================
Hits 15722 15722
Misses 1165 1165
Partials 300 300 ☔ View full report in Codecov by Sentry. |
8a54a38
to
1e83bb0
Compare
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.
Pinging @mx-psi on this as he tried this originally and had some comments around the readability of the code w/ this linter enabled
6947cd0
to
917efe3
Compare
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.
We should at least exclude the test files, there's not much of a point there in terms of perf improvements.
IMO it does hinder readability in structs like the ones on confighttp and configgrpc, where I would expect that the Endpoint/Address comes first in the struct. It also does not seem to be very beneficial there. See also the other issues I pointed out on #2789 (comment)
Maybe we can start with enabling it just for internal
packages? That seems like a good way to get started trying this out without worsening readability of the public API
I can take slices of this PR and apply them selectively to the codebase to follow the pattern your recommend as separate PRs. It will make review easier. |
Closing as this won't land as is. |
Fixes #2789