switch to diff format from REST API #26
Conversation
use git diff for anything that isn't a PR.
explicitly use commit hash in git show also be sure to strip leading '\n' from diff output (during parsing) switch from re.findall() to re.search() manually set fetch depth to 3 set depth to 3 via workflow
on local dev env, use git status --verbose
- move cli_arg_parser object into separate module - docs can now be built from src instead of installing the cpp-linter pkg
Since we're moving to using the diff format of REST API calls, these changes update unit tests that previously used the JSON format of REST API calls. Updated unit tests take longer to perform since we're now properly testing specific commits for multiple repos individually. note: the status field is no longer needed as the diff format provides more detail than the JSON format.
Cache the sample repo's downloaded files and copy to relavant test folder instead of re-downloading the same files for each test.
Codecov Report
@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 81.97% 83.52% +1.55%
==========================================
Files 6 8 +2
Lines 649 692 +43
==========================================
+ Hits 532 578 +46
+ Misses 117 114 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
- improve test coverage for get_list_of_changed_files() & git.py - remove unused function in docs/conf.py - restore test coverage of thread_comments.py - reduce nested if condition despite pylint warning
02129e3 to
5fc1743
Compare
|
is this PR ready to go? (just to make sure you're not waiting for me). looks like a big change. |
|
I was giving you time to review because it is a kinda big change, but it is well covered in the unit tests. Also, the news coming out of China sounds pretty serious, so take your time and try to stay safe. |
|
Yes, it's a big change, I'll take time to study, and I'm busy with work and my child. Hope it won't let you wait too long. |
|
|
|
"xfail" is short for "expected failure". So, yes "xfail" results are good. |
also update workflow matrix to include clang-tools v14 & v15
There was a problem hiding this comment.
Sorry for staying with me for so long. I think we can go ahead and do the final test with the action repo.
An immature comment from me maybe we can refactor run.py for easy reading in the future.
- personally I prefer main.py instead of run.py, it‘s easy to know that's the start of cpp_linter
- reduce the number of functions in run.py and put them in the different py files may be easier to understand
Btw, there is 
Yeah, I think some of the functions that use the REST API could go into its own rest.py module as well. I have no problem renaming run.py as main.py. Remember back when it was just a shell script named runchecks.sh?
It complains about a nest inline condition in cli.py. I chose to ignore it since it is used in a |
|
Fixed it. |
I like the idea of rest.py, maybe it can be refactored in the next(future) PR.
Yes, you have a good memory! |
| raw_url = f"https://github.com/{GITHUB_REPOSITORY}/raw/{GITHUB_SHA}/" | ||
| raw_url += urllib.parse.quote(file["filename"], safe="") | ||
| logger.info("Downloading file from url: %s", raw_url) | ||
| Globals.response_buffer = requests.get(raw_url) | ||
| # retain the repo's original structure | ||
| Path.mkdir(file_name.parent, parents=True, exist_ok=True) | ||
| file_name.write_text(Globals.response_buffer.text, encoding="utf-8") |
There was a problem hiding this comment.
This may not work on a submodules' srcs.
This may not be fork friendly in case someone runs the CI on a renamed fork.
f"https://github.com/{GITHUB_REPOSITORY}/raw/{GITHUB_SHA}/"
GITHUB_REPOSITORYwill always be set to the name of the repo running the CI.GITHUB_SHAwill always be set to the commit specific of the repo running the CI.
By default, we ignore submodules. But if the CI didn't checkout submodules and a submodule was explicitly not ignored, this code will get hit. When we get an incorrect GET response (because the raw_url was wrong), then there may be undefined behavior:
file_name.write_text(Globals.response_buffer.text, encoding="utf-8")
There was a problem hiding this comment.
On second thought, I don't think the diff will include changes to submodules' srcs. So, this should never be a problem.
And I think the fork-friendly problem might resolve itself because the specific commit should be inherited with the git history of the forked repo.
|
This CI hit the REST API rate limit for unauthenticated requests. https://github.com/cpp-linter/cpp-linter/actions/runs/3663491831/jobs/6193307913#step:8:251 This is because the |
|
All tests appear to have passed, it should be good to merge.
|
So will the next run fail all because of this? Btw, another 1 Code Smell was reported :) |
|
yeah, I can refactor that logic. I pushed rather hastily... |
|
yeah I think another CI run on this branch will fail. I need to monkeypatch the test. |
5490ae6 to
8630564
Compare
8630564 to
f59f661
Compare
|
Ok. I fixed the code smell issue by factoring out the logic to a private function. I also adjusted some unit tests that were directly using the REST API, so we shouldn't get those CI failures when the API rate limit is hit. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
👍 I also rerun the tests, they all passed. all looks good. |
resolves #10 (again)
partially resolves #4 (in a compromising way)
Doc updates
cli_arg_parserobject moved to its own module.std:optionbuilt into Sphinx).API changes
All diff parsing happens in
get_list_of_changed_files()which uses functions from the new git.py module. This means thatfilter_out_non_src_files()doesn't need to check for thestatusorpatchfields that were used in the JSON format of REST API calls.If executed from a local developer's environment,
get_list_of_changed_files()will use the output provided bygit status -v(with the summary info prelude stripped away). This should allow using options likelines-changed-onlyandfiles-changed-onlywithout needing a CI environment.unit test updates
It was getting pretty messy to test multiple commits from different repos that were defined in a hacky event_files.json file. We now have separate test resources to individually test multiple repos for multiple commits.
Furthermore, all tested repos' files are downloaded to the temporary test folder that is managed by pytest (as opposed to gitignoring the downloaded files in our
tests/capture_tools_outputfolder). To retain test performance, I employed a caching tactic to avoid re-downloading the same files for similar tests.