-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat(linter): add a check for affected field #295
Conversation
d9c9d59
to
fcc82cf
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.
This LGTM as is, all of my feedback is on naming.
) | ||
|
||
var CheckAffectedFieldValid = &CheckDef{ | ||
Code: "A0001", |
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.
(Over) thinking about codes, and prefixes, IMO, this is a record-level check, but I'm already using "R" for ranges...
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 also feel I shouldn't name this file as affected.go
?. record.go
sounds better and can add more checks in the future.
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 renamed the file to record.go
and renamed the check to CheckRecordHasAffected
, but i didn't change the code. I agree that this is a record-level check and "R" is better than "A". Should we rename the range code to "V" for versions?
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.
Should we rename the range code to "V" for versions?
Versions are one of the types of things in ranges :-)
Maybe for record-level, we go with the very non-obvious "O" for OSV? Naming is hard...
Signed-off-by: Holly Gong <[email protected]>
Signed-off-by: Holly Gong <[email protected]>
96a5a26
to
889b6bd
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.
LGTM, this is good enough for now. We can worry about improving the code later (famous last words)
affected:
should be defined in the data source and should not be an empty array.