-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add convenience functions for ErrorsCount
, WarningsCount
, Errors
andWarnings
#392
Conversation
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.
As said verbally, the only thing missing is a CHANGELOG entry and I left a nitpick/suggestion regarding the count methods.
🚀
diag/diagnostics.go
Outdated
func (diags Diagnostics) ErrorsCount() int { | ||
count := 0 | ||
|
||
for _, d := range diags { | ||
if SeverityError == d.Severity() { | ||
count++ | ||
} | ||
} | ||
|
||
return count | ||
} | ||
|
||
// WarningsCount returns the number of Diagnostic in Diagnostics that are SeverityWarning. | ||
func (diags Diagnostics) WarningsCount() int { |
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.
Nit: both those *Count
methods could be implemented as a len(Call_to_non_Count_methods)
.
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.
Good point. Fixed.
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.
🚀
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.
Code-wise I think this looks good, although I'd love to see/hear about expected use cases before expanding the public API here.
.changelog/392.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:feature |
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.
Nit: Typically features are "big" features whereas enhancements cover everything else
```release-note:feature | |
```release-note:enhancement |
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.
Fixed
expected diag.Diagnostics | ||
} | ||
tests := map[string]testCase{ | ||
"errors": { |
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.
Nit: Might be good to check what happens when:
- There is a
nil
diag.Diagnostics
, it should return nil or emptydiag.Diagnostics
- There is an empty
diag.Diagnostics
, it should return emptydiag.Diagnostics
- There are only warning diagnostics, it should return empty
diag.Diagnostics
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.
Added additional test coverage.
expected diag.Diagnostics | ||
} | ||
tests := map[string]testCase{ | ||
"errors": { |
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.
Nit: Might be good to check what happens when:
- There is a
nil
diag.Diagnostics
, it should return nil or emptydiag.Diagnostics
- There is an empty
diag.Diagnostics
, it should return emptydiag.Diagnostics
- There are only error diagnostics, it should return empty
diag.Diagnostics
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.
Added additional test coverage.
expected int | ||
} | ||
tests := map[string]testCase{ | ||
"errors": { |
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.
Nit: Similar to the Errors
testing, I think we should cover some additional cases just to ensure it returns 0
instead of panics, etc.
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.
Added additional test coverage.
expected int | ||
} | ||
tests := map[string]testCase{ | ||
"errors": { |
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.
Nit: Similar to the Warning
testing, I think we should cover some additional cases just to ensure it returns 0
instead of panics, etc.
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.
Added additional test coverage.
One use-case is in the instance where we're running |
To add to the "use cases": it's useful when writing tests to count the expected errors, instead of just checking |
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.
Looks good to me 🚀
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes: #391
Adds the following convenience functions to Diagnostics:
ErrorsCount()
WarningsCount()
Errors()
Warnings()