-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Implement updated pull request comments (HTML and MD) #578
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #578 +/- ##
============================================
+ Coverage 93.57% 93.74% +0.16%
- Complexity 583 602 +19
============================================
Files 49 49
Lines 2009 2110 +101
Branches 234 244 +10
============================================
+ Hits 1880 1978 +98
- Misses 59 60 +1
- Partials 70 72 +2 ☔ View full report in Codecov by Sentry. |
@@ -362,7 +338,7 @@ public void commentResults( | |||
webhookConfig.getHeadCommitSHA(), | |||
GHCommitState.SUCCESS, | |||
null, | |||
"No license issue detected", | |||
"No license issue(s) detected", |
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.
In the above code, there is message: "No license issue detected" (line 335). With this message, they are a little bit different. But in the above code, there are messages:
"Potential license problem(s) detected" (line 323)
"Potential license problem(s) detected" (line 329)
So maybe it will be more consistency to use the same style for all messages: "No license(s) issue detected"
Also, another question, is it normal that we use "problem" in one case and "issue" in another?
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.
Thank you for the comment. I've updated all messages.
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.
Great improvement!
.append("\n") | ||
.append(generateLicenseTable(detectedLicenseInfo, webhookConfig, vcs)) | ||
.append("\n"); | ||
} |
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.
maybe add else { log.error ("empty scan result"} ?
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.
It's normal when scanResults = null (no licenses detected: 95% of all scans have such a situation).
@tiokim Do you have any comments regarding this PR? |
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.
Great work, approved!
Signed-off-by: Oleg Kopysov <[email protected]>
Signed-off-by: Oleg Kopysov <[email protected]>
5a14523
to
c391e04
Compare
Pull Request
Description
The current PR contains significant updates on the pull request comments.
Changes cover HTML and MD comment formats.
Type of change
Please delete options that are not relevant.
Testing
Tested on the local environment.
Test PR: o-kopysov#2
Short view:
Full view:
Checklist: