-
Notifications
You must be signed in to change notification settings - Fork 24
Unit tests + coverage reports, type checking, pre-commit, and requested fixes #81
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
Conversation
- fix a few long line warnings - add rich to the py dev reqs - add types-PyYAML to dev reqs
also make sure the current working directory is appended to the .gitmodules path
- add pytest to dev reqs - rename run-pylint.yml to run-dev-tests.yml - install cpp-linter pkg in run-dev-tests.yml workflow - upgrade actions/setup-python to v2 in run-dev-tests.yml workflow - rename step in run-dev-tests.yml workflow
This allows the `lines-changed-only` option to accept the following values: - false: All lines in a file are analyzed - true: All lines in the diff are analyzed (including unchanged lines) - strict: Only lines in the diff that contain additions are analyzed gitignore some test resources as they are fetched running the tests. Retain original repo structure when fetching files. This is only used when the user doesn't checkout the repo being analyzed.
also fix a typo in demo.hpp variable name
The file annotations seem to be in order also (see here). I'm much more confident now that we have 80% coverage and expected behavior in the unit tests.
Could you also please update README.md to introduce v2 in this PR or #83. I believe your introduction must be better than mine. Thank you.
I think the v2 should go in #83 All the changes here were made with backward compatibility in mind. I do think there's some changes that should be made regarding the input options, so I'll re-read that file for accuracy.
Sure, please go ahead with #83
Is the .ci-ignore file still needed?
Lines 1 to 2 in a178730
I recall this was for an external linter CI you had configured for the repo, but not sure if changing to an org account affected that.
I forgot it was configured by me. I even don't know what it used for, I think we don't need it
I think I'll leave this alone until I'm sure the pkg is available on pypi
Line 185 in a178730
run: python3 -m pip install git+https://github.com/cpp-linter/cpp-linter-action@v1
It would be nice to have all versions of the python source code up on pypi, but that's a lot of work for code that is known to be flawed.
Please look over the readme changes. I also removed that .ci-ignore file.
All good to go on my end. Here's hoping uploads to pypi don't have the same problems we experienced with cpp-linter/clang-tools-pip v0.3.0 release.
🤞🏼
I added the PyPI token to this repo...
Why not add PyPI token to cpp-linter repo, do you want to upload the package from this repo first and then migrate it to cpp-linter?
I used the same approach(in clang-tools-pip) and upload cpp-linter/cpp-linter-hooks to PyPI, everything goes well.
The only problem is you need to create a token (the scope is for all projects) for uploading if you don't have the project named cpp-linter in your pypi, then you have to create another token that scope is only for cpp-linter and update the token in the GitHub repo if you care about security.
do you want to upload the package from this repo first and then migrate it to cpp-linter?
Yes. That would eliminate some guesswork from migration. PyPI doesn't care about the origin as long as the right credentials are used.
According to the codecov action, the secret token isn't needed for public repos.
OK, please go ahead to remove the token.
Done.
If you're using the upload script from codecov's instruction on the cpp-linter-hook repo, then switching to the codecov action will eliminate the need for that token in the org secrets.
I'm honestly not sure if the token is needed after uploading the first report.
I remember I copied the upload script and token from the codecov portal. It's Okay to remove the token and replaced with action, it should be easy to fix if any errors report.
Done (with no problems).
Done (with no problems).
So ready to merge?
yep
* add a submodule and update the CI checkout steps * Use NMake generator on Windows to get a DB * setup VS dev env in CI * don't use database option when running as action * do thread comments when run as action
Uh oh!
There was an error while loading. Please reload this page.
Issues addressed
ignoreoption's CLI default value includes repo root #75files_changed_onlyis True #76databaseoption in the docker env is currently discouraged (until we can switch to a composite action).github/workspacethat leads to a inaccessible absolute path. This is preventing any compilation database from being properly used in the docker env.databaseoption is resolved into an absolute path (if it isn't already absolute). Thanks to @BurningEnlightenment for the suggestion!And more
styleoption is set to a blank string ('')versionoption. Previously, this was intended only for Windows OS, but it was needed to allow custom builds of the tools to be executed from a user-defined install path.pathlibinstead ofosfor file system operations.pathlib.Path.rglob()(instead of usingos.walk()), I was able to remove a slightly misleading debug statement as well.