feat: add linter for error/trace/log messages #102
Conversation
There was a problem hiding this comment.
I'm going to wait to review the rest of the actual linter code until that one big comment about test/generated files still being linted against is addressed because I have a feeling it might change the structure of that linter's code a bit. It is definitely on its way and looking good though!!
|
Oh also - if I don't say it then @jkinkead will haha, please write a few test (unit) cases to validate your linter is working as intended. |
internal/config/tiers.go
Outdated
| @@ -164,7 +164,7 @@ func (l *Lintroller) EnsureMinimums(desired *Lintroller) error { //nolint:funlen | |||
| l.Doculint.MinFunLen = desired.Doculint.MinFunLen | |||
| } else if l.Doculint.MinFunLen > desired.Doculint.MinFunLen || l.Doculint.MinFunLen < 0 { | |||
| return fmt.Errorf( | |||
| "deviation detected from tier minimum defaults in lintroller.doculint.minFunLen, minFunLen must be set within (0, %d]", | |||
| "deviation detected from tier minimum defaults in lintroller.doculint.minfunlen, minfunlen must be set within (0, %d]", | |||
There was a problem hiding this comment.
internal/errorlint/errorlint.go
Outdated
| for _, ch := range msg { | ||
| if unicode.IsUpper(ch) { | ||
| isCap = true | ||
| } | ||
| } |
There was a problem hiding this comment.
shouldn't you just be checking the first character for capitalization?
There was a problem hiding this comment.
+1 to George's comment. This should only check the first letter.
There was a problem hiding this comment.
So this is where maybe other people's standards and ORCS standards diverge. We make the entire error message lowercase. What is the value in only the first letter being lowercase?
There was a problem hiding this comment.
the things that come to my mind immediately are acronyms (ASCII, etc) and abbreviations (ID, etc) and initialisms (UUID, etc)
There was a problem hiding this comment.
FWIW i do see where you're going though if we forget about what i just mentioned above @clevelittlefield-outreach
There was a problem hiding this comment.
yeah people resist on the acronyms, but the whole goal of this is to always find that error in the code/logs and not worry about casing, the first letter alone doesnt provide value
There was a problem hiding this comment.
i understand where you're coming from. i'm not the owner of this repo anymore (i just know the most about it) and im not on DT, so ultimately I'm going to defer this one over to @jkinkead
There was a problem hiding this comment.
I love the idea of having standardized style. I don't love the idea of having bespoke standardized style. For a widely-used linter, we should be adhering to established third-party standards.
Google's style guide (what I suggest we use, and what is linked in the linter) says:
Error strings should not be capitalized (unless beginning with an exported name, a proper noun or an acronym) and should not end with punctuation. This is because error strings usually appear within other context before being printed to the user.
It further says:
On the other hand, the style for the full displayed message (logging, test failure, API response, or other UI) depends, but should typically be capitalized.
I don't think this should deviate from this style guide, unless we can find an official Golang one that says something different.
Teams that want a different style that's still compatible with this (e.g. all characters lowercase), that's fine, but I don't think the linter should enforce that rule.
There was a problem hiding this comment.
So that exception will be hard to codify (unless beginning with an exported name, a proper noun or an acronym), and I dont think we want to have people do nolints to enforce that exception. That rule (not uppercase for first character only) honestly seem like arbitrary aesthetics rather than serving a clear purpose.
I do not understand the further part, we have one string here, how can it be all capitalized in another context?
Beyond that, I swore that gobox (back in go-outreach days) at one time in the readme asked for all lowercase errors. Using that starter guidance, but also building around that idea, is we want all strings of this sort to be able to be found going from code to datadog and from datadog to code. One of those at the time was case sensitive. I would have to test again to see if that is still the case. The point of leaving out casing it so avoid those accidental misses due to case.
If we decide this linter is not right for all of outreach, that is ok. I questioned that when we said it could be outreach wide in the first place because I have seen plenty of code that violates that. We will also have lots of violations when we build a linter for this guideline as well. Which circles back to the original question, how can we build and include in the ruleset a linter just for just the ORCS team?
internal/errorlint/errorlint.go
Outdated
| for _, ch := range msg { | ||
| if unicode.IsUpper(ch) { | ||
| isCap = true | ||
| } | ||
| } |
There was a problem hiding this comment.
+1 to George's comment. This should only check the first letter.
internal/errorlint/errorlint.go
Outdated
| for _, ch := range msg { | ||
| if unicode.IsUpper(ch) { | ||
| isCap = true | ||
| } | ||
| } |
There was a problem hiding this comment.
So this is where maybe other people's standards and ORCS standards diverge. We make the entire error message lowercase. What is the value in only the first letter being lowercase?
internal/errorlint/errorlint.go
Outdated
| for _, ch := range msg { | ||
| if unicode.IsUpper(ch) { | ||
| isCap = true | ||
| } | ||
| } |
There was a problem hiding this comment.
I love the idea of having standardized style. I don't love the idea of having bespoke standardized style. For a widely-used linter, we should be adhering to established third-party standards.
Google's style guide (what I suggest we use, and what is linked in the linter) says:
Error strings should not be capitalized (unless beginning with an exported name, a proper noun or an acronym) and should not end with punctuation. This is because error strings usually appear within other context before being printed to the user.
It further says:
On the other hand, the style for the full displayed message (logging, test failure, API response, or other UI) depends, but should typically be capitalized.
I don't think this should deviate from this style guide, unless we can find an official Golang one that says something different.
Teams that want a different style that's still compatible with this (e.g. all characters lowercase), that's fine, but I don't think the linter should enforce that rule.
What this PR does / why we need it
This PR adds linter for error/trace/log messages. This linter checks following-
JIRA ID: XX-XX
Notes for your reviewers
TODO: test cases