-
Notifications
You must be signed in to change notification settings - Fork 21
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
Shouldn't we be checking all files in the repo? #25
Comments
It’s best to help users check or output prompts to tell them that they need to have If their code has a submodule or subtree, they should also check out if they need it. cpp-linter-action only focuses on the work of clang-format and clang-tidy. So if the repo was checked out beforehand, there is no need to download the source code through the REST API, right? |
I did alter the example yml in the readme with a comment about this. I do like the idea of having something in the log about non-existent files. Currently, all we're logging about this is
but it should say more to make the user aware that the action is trying to recover from no git checkout.
True, but it doesn't hurt too much to leave it in as a recourse.
Checking out submodules is often done for complex projects' build CI. Furthermore, the compiler database file that clang-tidy looks for is generated at build time (ie using |
example of a .gitmodules file[submodule "RF24"]
path = RF24
url = https://github.com/nRF24/RF24.git
[submodule "RF24Network"]
path = RF24Network
url = https://github.com/nRF24/RF24Network.git
[submodule "RF24Mesh"]
path = RF24Mesh
url = https://github.com/nRF24/RF24Mesh.git
[submodule "pybind11"]
path = pybind11
url = https://github.com/pybind/pybind11.git The paths are relative to the repo's root (which can be specified using the input option |
I added a conditional warning in the log about missing files when no git checkout was used. |
I mean we don't need to consider submodules, users should consider it by themselves. The working process of cpp-linter-action is to have a complete code, then to analyze the specified code changes and post results comments. I don't know if I misunderstand your comment about submodules. |
Tested and saw the warning looks good in log |
Yeah I think there's a misunderstanding. I'm trying to say the same thing you are: Don't pay attention to submodules. Although I keep going into detail about how to identify if a path in the repo is a submodule (using .gitmodules). The important thing I'm questioning is: Should we examine all source files or only the files that changed?Currently, we are only examining the files that changed.
I think I'll start another branch on my fork to see what this looks like. |
I think I see. most CI checks are only checking the changed files, such as SonarQube Quality Gates. I would like to set |
The way I have it setup now (in master-py branch): If we set the |
Oh... |
The |
In GitHub, the diff files button called "Files changed", maybe we name them |
I like your rationality. I'm going with your name ideas. TBC... |
Quick Update (not committed yet)I was easily able to implement the The exclusion of submodules when - uses: actions/checkout@v2
- uses: shenxianpeng/cpp-linter-action@master
id: linter
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
style: file
# search entire repo for source files
file-changed-only: false
# ignore just the demo.cpp file in the demo folder (demo.hpp is still checked)
ignore: "demo/demo.cpp" Using log output commandsI came across a special command in the github action docs that basically interprets this print("::group::Section Name")
print("some descriptive output")
print("::endgroup::") and renders it in the action's logs like so
Amazing discoveryAfter looking into the other available action log commands, I found a way to directly annotate the checked source files with this action's output. This should look like the notification that the code-inspector CI outputs.
With these discoveries, posting comments in the file's diff (or even in files that aren't changed) should be much less problematic than going through the REST API! 🥇 |
Great discovery, log output command looks really good 💯 |
I'm close to submitting a PR, but I have to refine handling the new My current idea: - uses: actions/checkout@v2
- uses: shenxianpeng/cpp-linter-action@master
id: linter
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
style: file
# search entire repo for source files
file-changed-only: false
# ignore everything in demo folder except the demo.cpp file. Notice the bang (`!`) prefix
ignore: |
# yml comments are not recognized here
demo
!demo/demo.cpp You can find my progress in this latest run |
Your idea and latest run look good to me 👍 |
|
I tried out using the new log commands to implement in-file comments, but I found some differences compared to using the REST API to post comments.
However, despite these differences, it is much easier (and even faster) to use log commands. What you think?
|
Making decisions is difficult. I prefer the last one to have both. I have seen a few Lint and other Actions with many starts. Most of them support log commands, and few use REST API to add comments. It seems super-linter only support log commands codecov-action has both
If there are both, it might be better to have a switch to control log command and REST API comment on or off. |
oh I forgot about the retention period! We shall have both then. PR coming soon... |
The link to super-linter example is a year old, but the annotations still exist 👍🏼
This thought had occured to me also. How about a new option called I also think we should switch away from using a table in the readme to document the input options. They are getting numerous, and the |
Do you mean to call 2bndy5@7ce1dd6 as If there are other better ways to show input options, please go ahead. Btw, I still like using readme to display the basic usage. |
I mean the comments that the action currently uses thread comments. I thought to call them thread comments instead of having "commit comments" and "PR comments" options. You are correct; the picture you posted are thread comments. The output created from the log commands are called (by github) "annotations". Its a pretty word for saying "notes with various priorities". The readme can stay as is, but instead of using a table to show all input options, I want to use nested & unordered lists. Trust me the information for the ignore option will be hard to read in the cell of a table. |
* initial attempt (#25) * change test action to my branch * adjust workflow triggers * try log grouping with logger.critical() * change logger's format * don't upgrade pip; test some new features * test fake submodule; show me args.ignore on boot * considering alternate fmt for ignore option * remove fake submodule (it worked); * action inputs can't take a sequence; delimit by \n * specifying `--ignore` better * fix exit early when no files found * list_source_files() is malfunctioning??? * Revert "list_source_files() is malfunctioning???" This reverts commit 59522e0. * is ignore option causing malfunction? * show me what paths are being skipped * show me which paths are crawled * show me which files are considered as src files * show me comparison of ignored paths * change done debug statements * fix debugging statement in is_file_ignored() * skip comparing empty strings in ignored paths * maybe a bug about ignoring root dir * try getting python latest from src * python src build needs deps. revert to apt build * try using tojson() * bad yml fmt * try a different json approach * make yml array an explicit str * try forcing it as a py list * abandon json idea * try new log_cmder and !ignore prefix * need to separate a single str into multi args * use pipe char as delimiters * slightly different approach to passing ignore val * might need to switch to 1 line of input * how to handle spaces in a path among multiple * no need to escape quotes * fix debug prompts; and workflow-triggered paths * fix cpp-linter-test action's triggered paths * check action stil works without using new features * try log cmds to annotate * try abbotations again * use a long unlikely string as default ignore * try annotating with line and columns * try \n with minimal parameters * does file need changes to show annotation? * use html <br> and line's columns * trigger annotations * adjust annotation's output * try CRLF * simply can't use mult-line annotation msg * almost ready for PR * use proper casing in chosen style name * adjust last commit for GNU style as well * remove artifact * we don't need the `re` module anymore * new thread-comments option; update docs & README * avoid duplicate clang-tidy comments * [no ci] proofread README * Fix indent in root README for mkdocs * Use sub-headings instead of list points in README * reviewed errors in docs * switch test workflow to upstream action
Close it as it has been resolved in #26 👍 |
* Parse better with python (#22) * upload new demo image * Revert "upload new demo image" This reverts commit 1aff6c7. * update readme & upload new demo pic * avoids duplicated checks on commits to open PR * fix workflow from the last commit * Revert "fix workflow from the last commit" This reverts commit 778e10d. * create python scripts * output event payload & pip3 ver * echo pip3 list * switch action to this branch * echo pip3 list * install pip and list modules * typo * pip3 install requests * switching to only python * should've read the docker docs * switching back to ENTRYPOINT * typo * don't upgrade if files exist outside /use dir * use python as an entrypoint * add project.toml * bad syntax * try again * hard_code version in setup.py * try pip install * only upgrade pip * test on source files * headers are dicts * test on source files * fix posting comment using requsts * fix passing verbosity to CLI args * action uses current branch for now * parsing everything; no diff action yet * show all debug in logs * use bot's id not mine * double trigger action * auto-verbose logging on repeated runs * show me then run number * support sync events in PRs * switch to mkdocs * fix bad indent in yml * install pkg before documenting * typo * compatible w/ windows; add diff-only input option * diff comments working on PR from clang-tidy advice * rolling back diff comments; update docs * use bot id * disable diff-only in demo * Update setup.py * try mkdocs gh-deploy * use a gh action to publish docs * oops. ignoe release only condition for now * change doc's favicon * [no ci] publish docs only on release event * rename docs CI workflow * prototype badge * [no ci] augment doc build instructions * update readme & demo picture * [no ci] pub docs on release * update docs * disable mkdocs CI (switching to rtfd CI) * fix some review suggestions * update badge in readme * Use lazy % formatting in logging functions * fix more code-inspector notices * slight refactor and switch to pylint * fix pylint workflow * run pylint on PR synch events too * Tell code-inspector to ignore python srcs * Tell code inspector to ignore mkdocs.yml * ran black * self review changes * Update .github/workflows/run-pylint.yml * add gitpod badge to root README.md * try to fix verify_files_are_present() * remove <br> to auto adjust and easy copying * fixed typo * solution to #24 * using check=True causes #24 * log non-zero exit codes as warnings * warn (in log) about no git checkout * Update action name Update the name to make it easier to search in the marketplace when users search with clang-format or clang-tidy. * Revert "Update action name" This reverts commit 4a41a5e. * Upadate README.md * Check all files (#26) * initial attempt (#25) * change test action to my branch * adjust workflow triggers * try log grouping with logger.critical() * change logger's format * don't upgrade pip; test some new features * test fake submodule; show me args.ignore on boot * considering alternate fmt for ignore option * remove fake submodule (it worked); * action inputs can't take a sequence; delimit by \n * specifying `--ignore` better * fix exit early when no files found * list_source_files() is malfunctioning??? * Revert "list_source_files() is malfunctioning???" This reverts commit 59522e0. * is ignore option causing malfunction? * show me what paths are being skipped * show me which paths are crawled * show me which files are considered as src files * show me comparison of ignored paths * change done debug statements * fix debugging statement in is_file_ignored() * skip comparing empty strings in ignored paths * maybe a bug about ignoring root dir * try getting python latest from src * python src build needs deps. revert to apt build * try using tojson() * bad yml fmt * try a different json approach * make yml array an explicit str * try forcing it as a py list * abandon json idea * try new log_cmder and !ignore prefix * need to separate a single str into multi args * use pipe char as delimiters * slightly different approach to passing ignore val * might need to switch to 1 line of input * how to handle spaces in a path among multiple * no need to escape quotes * fix debug prompts; and workflow-triggered paths * fix cpp-linter-test action's triggered paths * check action stil works without using new features * try log cmds to annotate * try abbotations again * use a long unlikely string as default ignore * try annotating with line and columns * try \n with minimal parameters * does file need changes to show annotation? * use html <br> and line's columns * trigger annotations * adjust annotation's output * try CRLF * simply can't use mult-line annotation msg * almost ready for PR * use proper casing in chosen style name * adjust last commit for GNU style as well * remove artifact * we don't need the `re` module anymore * new thread-comments option; update docs & README * avoid duplicate clang-tidy comments * [no ci] proofread README * Fix indent in root README for mkdocs * Use sub-headings instead of list points in README * reviewed errors in docs * switch test workflow to upstream action * Update the new example yml and remove old demo link Co-authored-by: Brendan <[email protected]>
This occurred to me when I was adding the
diff-only
option. Ifdiff-only
is false, then shouldn't we be examining all source files in the repo that use one of the designated fileextensions
? Currently, we are only examining files that have been changed despite whatdiff-only
option is set to.However, we can only do this if the repo is checked out before this action is run, but there's the concern of clever people also checking out their repo's submodules (which shouldn't be examined by this action). In case the repo is setup to use submodules, we can use the .gitmodules file to ignore any existing submodules. This wouldn't be hard at all since the .gitmodules file uses an ini file syntax which python's std configparser module can handle nicely.
I suppose we can use the REST API to list all files in the repo, but the REST API's responses are limited to 30 entries per page. If the repo has more than 30 files, then we'd have to subsequently keep making REST requests until we have a list of all the files in the repo. It would definitely be easier if the repo was checked out beforehand, and we use python to "walk" through the repo's files and examine any applicable source files.
The text was updated successfully, but these errors were encountered: