submodules are only ignore if `files_changed_only` is True

[2bndy5]

I noticed this on a repo I'm developing that has 4 submodules. When the CI runs

cpp-linter --files-changed-only=false

All the submodules are crawled as if they're part of the current repo's original sources.

Moving the algorithm that detects submodules from list_source_files() into parse_ignored_option() should fix this, but I have to be careful to consider if the user explicitly specifies a submodule to be not ignored.

[shenxianpeng]

When --files-changed-only=ture, it should ignore submodules.
When --files-changed-only=false, analyze any source files in the repo including submodules.

So the current behavior of --files-changed-only=false is not expected?

I'm wondering if we should change files-changed-only default value to true. I guess users only care about modified files, not all source files.

[2bndy5]

When --files-changed-only=false, analyze any source files in the repo including submodules.

This was not my original intention. It doesn't make sense (to me) for a linter to be concerned with external sources. That's why in the README, I noted the ignore option can be used to explicitly include submodules (just not hidden folders that begin with a . like .github).

I'm wondering if we should change files-changed-only default value to true. I guess users only care about modified files, not all source files.

I have no objections to this. I think this was the original default until we started encouraging users to have a actions/checkout step. I also expect most users only want the file changes analyzed. Personally, I've been using this action to check all sources in my projects (for completeness because I itch for that 💯 ). I haven't been using clang-tidy for anything lately (but that may change in the future).

[2bndy5]

I'm almost ready to draft a PR... There's a lot of changes, so the description will be wordy. I'd like to learn a bit more about pytest (& plugins) to optimize the unit tests I've written.

[shenxianpeng]

It doesn't make sense (to me) for a linter to be concerned with external sources

I agree we should only analyze any source files and ignore submodules when --files-changed-only=false

No worries, take your time. I'm also learning about pytest to refactor my bad smell unit tests :(

[2bndy5]

I might have to use a matrix in the CI to test various versions of python with various versions of clang-tools on various OSs... My new database test is failing in CI, but it passes fine locally.

[shenxianpeng]

It also failed in my local environment.

/home/ubuntu/.local/lib/python3.8/site-packages/requests/adapters.py:501: ConnectionError
--------------------------------------------- Captured log call ---------------------------------------------
WARNING  CPP Linter:run.py:340 Could not find pcsound/pcsound.c! Did you checkout the repo?
========================================== short test summary info ==========================================
FAILED tests/lines_changed_only/test_lines_changed_only.py::test_run_clang_format_diff_only - requests.exc...
FAILED tests/lines_changed_only/test_lines_changed_only.py::test_run_clang_format_diff_adds - requests.exc...
================================= 2 failed, 22 passed in 257.41s (0:04:17) =================================

It's hang and failed from tests/lines_changed_only/test_lines_changed_only.py::test_run_clang_format_diff_only

tests/ignored_paths/test_ignored_paths.py::test_not_ignored PASSED                                    [ 83%]
tests/ignored_paths/test_ignored_paths.py::test_ignore_submodule PASSED                               [ 87%]
tests/lines_changed_only/test_lines_changed_only.py::test_lines_changed_only PASSED                   [ 91%]
tests/lines_changed_only/test_lines_changed_only.py::test_run_clang_format_diff_only 

[2bndy5]

It failed locally for you because it couldn't download the source files from the commit that I'm using in the test.

The test that failed for you was the lines-changed-only test. The one that failed in CI was the database test.

[2bndy5]

I've been playing with test coverage to see what else needs to be tested. So far I'm (locally) getting 94% coverage if I exclude any functions that directly use the REST API for thread comments:

I also configured coverage.py to ignore code specific to type checking and anything related to using rich as a logging extension, thus __init__.py is 100% covered. cpp_linter/thread_comments.py is also ignored because we would need a real CI event to test that code.

@shenxianpeng I see you've already created a codecov token for cpp-linter/clang-tools-pip. Can you create a org secret with a new codecov token, so we can use it for multiple repos? Or does the token have to specific to a project?

ps - Ensuring the best coverage has caused further improvements to the code base.

[shenxianpeng]

The test coverage is already very high 👍

@2bndy5 I have created a org secret with a new codecov token and also removed codecov token for cpp-linter/clang-tools-pip, then rerun the test action of clang-tools-pip, it still works.