Add CodeScanningService.ListAnalysesForRepo and CodeScanningService.GetAnalysis #2210
Conversation
|
I just submitted a CLA. |
|
I noticed that most of the unit tests are prefixed with "TestActionsService", shouldn't these unit tests be prefixed with "TestCodeScanningService" instead? |
Codecov Report
@@ Coverage Diff @@
## master #2210 +/- ##
=======================================
Coverage 97.79% 97.80%
=======================================
Files 112 112
Lines 10036 10078 +42
=======================================
+ Hits 9815 9857 +42
Misses 154 154
Partials 67 67
Continue to review full report at Codecov.
|
Good catch! Yes indeed, these should be changed. Would you mind addressing these changes in this PR? @googlebot rescan. |
google-cla
bot
added
cla: yes
github/code-scanning.go
Outdated
| // Analysis represents an individual GitHub Code Scanning Analysis on a single repository. | ||
| // | ||
| // GitHub API docs: https://docs.github.com/en/rest/reference/code-scanning#list-code-scanning-analyses-for-a-repository | ||
| type Analysis struct { |
There was a problem hiding this comment.
Can this be merged with the existing SarifAnalysis struct?
If not, can we give it a more specific name?
There was a problem hiding this comment.
It can't because SarifAnalysis is used for POST request body params, while Analysis is used to unmarshal json response of listing code scanning analysis instances.
I thought about renaming to to ScanningAnalysis or CodeScanningAnalysis but I felt that was redundant with the service name. CodeScanningService. What are your thoughts on this?
There was a problem hiding this comment.
The problem is that this name is global throughout the package github namespace. So even though it is used within the CodeScanningService, it is really github.Analysis which is pretty general, so if you don't mind switching to ScanningAnalysis, I think that would be preferable. Thank you!
There was a problem hiding this comment.
Oh yes you're right. I updated it. Thanks for the review!
@gmlewis No problem! I committed the name change. |
There was a problem hiding this comment.
Thank you, @yogur !
LGTM.
Awaiting second LGTM before merging.
(Please note that ALL other contributors to this repo are welcome to provide the second PR review/comment/approval that we need for merging and that we are not waiting for any particular reviewer unless otherwise noted.)
|
Thank you, @cpanato ! |
Add ListAnalysesForRepo and GetAnalysis to CodeScanningService, including unit tests.