Hide outdated summary comments on GitHub #323
Conversation
VanRoy
force-pushed
the
github-hide-old-comment
branch
from
f3e626d
to
e3a4faa
Compare
|
Hi @mc1arke , do you think that this enhancement is useful for the project ? Do not hesitate to tell me if you think it require some changes , more tests or something else. Thanks a lot. |
| @@ -49,13 +51,81 @@ public PullRequest getPullRequest() { | |||
|
|
|||
| private final String id; | |||
|
|
|||
| @GraphQLProperty(name = "comments", arguments = {@GraphQLArgument(name = "last", value = "100", type = "Integer")}) | |||
There was a problem hiding this comment.
The default of 100 could cause problems, as an MR with a large number of comments would only have the last 100 checked. This should really be controlled from the calling code, with an additional field of totalCount on the response so we can iterate from start: 100 to start: totalCount in batches of 100 and ensure we've covered all messages
| @@ -222,6 +231,42 @@ private void postSummaryComment(String apiUrl, Map<String, String> headers, Stri | |||
|
|
|||
| } | |||
|
|
|||
| private void minimizeComment(String graphqlUrl, Map<String, String> headers, String commentId) { | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
Nitpick: the IOException can only happen on the executeRequest as far as I can tell, so keep the try block constrained to that line rather than covering the whole method.
| @@ -178,29 +180,36 @@ public DecorationResult createCheckRun(AnalysisDetails analysisDetails, AlmSetti | |||
|
|
|||
| } | |||
|
|
|||
| private void postSummaryComment(String apiUrl, Map<String, String> headers, String projectPath, String pullRequestKey, String summary) throws IOException { | |||
| void postSummaryComment(String graphqlUrl, Map<String, String> headers, String projectPath, String pullRequestKey, String summary) throws IOException { | |||
There was a problem hiding this comment.
If this has been made visible purely for testing, then it indicate this class has potentially got too complex. Is it worth moving this into a SummaryCommentHandler class?
| } | ||
| } | ||
|
|
||
| public static class Comments { |
There was a problem hiding this comment.
Nitpick: I'm not sure these classes need to be static nested classes of GetPullRequest. They are part of the response from GetPullRequest, but would also be valid for anything else that uses Comment nodes
|
Hi @VanRoy, really nice enhancement 👍, thanks for that. I think it's something that really missing right now. |
|
Merged under #435 with a few minor changes. Thanks for the contribution! |
This pull request enhance the support of the summary comment for GitHub by minimizing previous ( outdated ) comments on Pull Request when new commit is pushed on same branch and the analysis is changed.
Screenshot of comment minimized after re-analysis done by SonarQube :