Initial commit of the collect_signals command. #120
Conversation
- defines key APIs: - projectrepo.Repo and projectrepo.Factory - signal.Set and signal.Signal - collector.Collector and collector.Registry - implements the complete current set of signals for GitHub repos. - supports CSV output. - refactors GitHub API related code into an internal package for reuse.
|
Sorry for the huge commit. |
| "github.com/shurcooL/githubv4" | ||
| ) | ||
|
|
||
| type Client struct { |
There was a problem hiding this comment.
The consumers are directly using the GitHub clients. This is going to make it harder to unit test. The suggestion would be to use an interface similar to this https://github.com/ossf/scorecard/blob/7ac81a334f7ef5281d3238ffd68499a6a48106a8/clients/repo_client.go#L29 https://github.com/ossf/scorecard/blob/7ac81a334f7ef5281d3238ffd68499a6a48106a8/clients/repo.go#L18
There was a problem hiding this comment.
Thanks for the feedback, interfaces certainly help with testability and I do need to do some work in this area.
At the moment I can mock the HTTP libraries, but that may not be the most practical approach.
I expect this area to evolve significantly over the next few months.
| if comments, err := legacy.FetchIssueCommentCount(ctx, ghr.client, ghr.owner(), ghr.name(), legacy.IssueLookback); err != nil { | ||
| if errors.Is(err, legacy.TooManyResultsError) { | ||
| ghr.logger.Debug("Comment count failed with too many result") | ||
| s.CommentFrequency.Set(2.0) |
There was a problem hiding this comment.
nit: put 2.0 in a constant to make it clear this is the default?
Also, why 2.0 as the default?
There was a problem hiding this comment.
2.0 was roughly what I was seeing on repos (usually between 1.5 and 2.5) for comment frequency.
It is a bit of a magic value, and leaving it unset may be more accurate.
| cs, resp, err := c.Rest().Issues.ListComments(ctx, owner, name, 0, opts) | ||
| // The API returns 5xx responses if there are too many comments. | ||
| if c := githubapi.ErrorResponseStatusCode(err); 500 <= c && c < 600 { | ||
| return 0, TooManyResultsError |
There was a problem hiding this comment.
This seems inconsistent with the other implementations' behaviour (e.g. contributor count), which seems to hide this detail from the caller and just return the maximum number.
i.e.
cs, resp, err := c.Rest().Repositories.ListContributors(ctx, owner, name, opts)
if errorTooManyContributors(err) {
return MaxContributorLimit, nil
}Would that make more sense here too?
There was a problem hiding this comment.
I'm not a huge fan of this difference, but it currently exists this way due to the usage of the results from FetchIssueCount() to generate the frequency (i.e. average comments per issue).
I have an refactoring in mind to improve the API around a struct{} that could also be used to reduce the number of args to the methods and provide some caching - making it possible to move this into a more logical location.
For the time being I'd be happy to get this merged and do some future refactoring soon (I've added a TODO).
anyNot yet there: