Skip to content

Conversation

maxnoe
Copy link
Contributor

@maxnoe maxnoe commented Feb 16, 2024

I tried fixing #28, but this turned into a somewhat larger refactor, as I found a couple of more issues.

First: try-repo doesn't actually support running the current code and it doesn't read the config file and it doesn't support passing args. There are a couple of issues about that in the pre-commit repo, but I didn't find a really good solution.

Second: I could simplify the code a lot by using an ArgumentParser for the command line args and by using python functions from clang_tools instead of using subprocess to call the cli.

Let me know what you think.

@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Feb 17, 2024

Thanks so much for your fixing and refactoring.

Can you please review sonarcloud issues, my comments and also get CI to pass?

@shenxianpeng shenxianpeng self-requested a review February 17, 2024 10:25
@maxnoe
Copy link
Contributor Author

maxnoe commented Feb 19, 2024

The tests are failing on github actions now, because clang tools are already installed there, so the tests that assumed that when starting the tests, we don't have the tools fail.

Will think about how to actually test that the installation works.

@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Feb 28, 2024

Hi @maxnoe thanks again for your excellent changes. it's ok if you don't have a good solution for the test failure, you can skip it for now, although it might reduce code coverage, but we can record skip tests by creating another ticket.

@shenxianpeng shenxianpeng added the bug Something isn't working label Feb 28, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 74.74747% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 72.53%. Comparing base (54c21bd) to head (d7dca0d).

Files Patch % Lines
tests/test_util.py 37.50% 15 Missing ⚠️
cpp_linter_hooks/clang_format.py 71.42% 4 Missing ⚠️
tests/test_clang_tidy.py 63.63% 4 Missing ⚠️
cpp_linter_hooks/clang_tidy.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## main #29 +/- ## ========================================== - Coverage 77.84% 72.53% -5.31%  ========================================== Files 7 6 -1 Lines 176 142 -34 ========================================== - Hits 137 103 -34  Misses 39 39 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shenxianpeng shenxianpeng self-requested a review March 6, 2024 02:50
@shenxianpeng shenxianpeng merged commit a346670 into cpp-linter:main Mar 6, 2024
@shenxianpeng shenxianpeng added the minor A minor version bump label Mar 6, 2024
@maxnoe maxnoe deleted the fix_install branch March 6, 2024 10:12
@shenxianpeng shenxianpeng mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working minor A minor version bump

2 participants