-
-
Notifications
You must be signed in to change notification settings - Fork 8
resolve #34 #35
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
resolve #34 #35
Conversation
Codecov Report
@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 81.34% 84.37% +3.02%
==========================================
Files 8 8
Lines 713 691 -22
==========================================
+ Hits 580 583 +3
+ Misses 133 108 -25
|
Codecov CI is complaining about lines that changed in this PR but are not covered with unit tests. Honestly, we don't have an adequate way to test anything involving Github's REST API usage (not without hitting the REST API's usage rate limit). So, the codecov complaints are null because the functionality wasn't unit tested before anyway. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
The overall coverage went up, but the diff coverage is below some target that codecov seems to have set (81.48%). |
8a48cb1
to
423dbed
Compare
So far, I've tested the following configs: with:
files-changed-only: true | false
thread-comments: update
no-lgtm: false with:
files-changed-only: true | false
thread-comments: true
no-lgtm: false Now, I have to test the same |
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.
LGTM. nice work!
Code coverage is missing some lines in |
needs testing
allows LGTM comment in such a case fix unit test and comments.json debug output file
applied in make_annotations()
found a bug where file-annotations=false & tidy advice was still posted
TL;DR I need to find a better commit for the After playing around with |
also adjust unit test to use a src that triggers tidy annotations
I also found that the filename matching was flawed in |
Kudos, SonarCloud Quality Gate passed!
|
Everything passed on the second round of tests in cpp-linter/test-cpp-linter-action#22. I'm now confident enough to merge this! @shenxianpeng Thanks for your patience. |
This still needs testing.
Also updated the pre-commit hooks. I replaced pylint with ruff for better speed and because we already get type-checking from mypy; we don't need the extra type-checking from pylint.
Removed some dead code from run.py. I have some other optimizations I'd like to add from the deep-dive code review I did when porting this pkg to dart. But that can happen in another PR.
I found and fixed a couple bugs in
make_annotations()
:file-annotations
is false, then clang-tidy annotations were still posted.file-annotations
is false, then clang-format advice was not counted.I also modified the log output to show the count of possible annotations which directly corresponds to the
checks-failed
output.