feat(conflictingmarkers): Make error messages more precise #156
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
Updates the `conflictingmarkers` analyzer to include the struct name in the error message, changing the output from: "field <fieldName> has conflicting markers..." to: "field <structName>.<fieldName> has conflicting markers..." This provides more precise context, making it easier for developers to locate the source of the error, especially in complex types. The struct name is reliably determined by inspecting the AST stack during analysis.
yongruilin
force-pushed
the
conflictmarkers-more-detail
branch
from
d060b21 to
c117432
Compare
|
/assign @JoelSpeed |
| // Find the struct name from the stack | ||
| for i := len(stack) - 2; i >= 0; i-- { | ||
| if typeSpec, ok := stack[i].(*ast.TypeSpec); ok { | ||
| if _, ok := typeSpec.Type.(*ast.StructType); ok { | ||
| structName = typeSpec.Name.Name | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If we find that this is a valuable addition to the messages output by the linting checks, this should probably be a sweeping change across all the linting checks.
What do you think about turning this into a generic utility that can be used by existing and new linting checks that should be providing this additional granularity?
There was a problem hiding this comment.
Agreed, this seems like something we should agree to change globally (e.g. we already have something similar in conditions, but different) and maybe even make a standard utility that looks up the name from the pass rather than passing it all through
There was a problem hiding this comment.
I added a generic util func using pass and field to get the structname.
| } | ||
|
|
||
| func checkConflict(pass *analysis.Pass, field *ast.Field, markers markers.MarkerSet, conflictSet ConflictSet) { | ||
| func checkConflict(pass *analysis.Pass, field *ast.Field, structName string, markers markers.MarkerSet, conflictSet ConflictSet) { |
There was a problem hiding this comment.
Will this handle conflicting tags when applied to a tyoe definition (e.g. a string alias)?
k8s-ci-robot
added
size/L
Adds a new utility function, `GetStructNameForField`, to determine the parent struct name for a given field by inspecting the analysis pass. Refactors the `conflictingmarkers` analyzer to use this new utility, simplifying the analyzer and improving the detail in its reports.
Adds a new test case for the `GetStructNameForField` utility function. This test uses the `analysistest` package to run a dedicated analyzer over a test data file, ensuring the function correctly identifies the parent struct for a given field.
yongruilin
force-pushed
the
conflictmarkers-more-detail
branch
from
b8463cf to
b09f061
Compare
|
/approve Thanks for the update @yongruilin, I'll create an issue so that we follow up and make this consistent among all the analyzers in the future |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, yongruilin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
5 participants
Updates the
conflictingmarkersanalyzer to include the struct name in the error message, changing the output from:"field has conflicting markers..."
to:
"field . has conflicting markers..."
This provides more precise context, making it easier for developers to locate the source of the error, especially in complex types. The struct name is reliably determined by inspecting the AST stack during analysis.
This is for addressing the exception confusion: kubernetes/kubernetes#134229 (comment)