[Tools] move precommit checks to tools/#1781
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
There was a problem hiding this comment.
Code Review
This pull request migrates several pre-commit check scripts to a new tools directory and introduces a shamrock_tool_banner utility for standardized output. The review identifies critical bugs in the banner script, specifically an incorrect project path calculation and a typo in the git status logic. Further recommendations include replacing a bare except clause, using the subprocess module instead of os.popen, and eliminating redundant path calculations across scripts by leveraging the variables now provided by the banner utility.
| """ | ||
| # end allow utf-8 | ||
|
|
||
| abs_proj_dir = os.path.abspath(os.path.join(__file__, "../../..")) |
There was a problem hiding this comment.
The project directory calculation is incorrect. Using os.path.join(__file__, "../../..") on a file located in tools/ resolves to the parent of the project root, not the root itself. It should be two levels up from the file path to reach the root.
| abs_proj_dir = os.path.abspath(os.path.join(__file__, "../../..")) | |
| abs_proj_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) |
|
|
||
| try: | ||
| col_cnt = os.get_terminal_size().columns | ||
| except: |
| git_head = "\033[1;92m" + git_head[0] + "\033[0;0m" | ||
| else: | ||
| git_head = ( | ||
| "\033[1;92m" + git_head[0] + "\033[0;0m , \033[1;91m" + git_head[0] + "\033[0;0m" |
There was a problem hiding this comment.
There is a typo here where git_head[0] is used twice. When multiple references are present in the git log decoration, the second one should likely be git_head[1].
| "\033[1;92m" + git_head[0] + "\033[0;0m , \033[1;91m" + git_head[0] + "\033[0;0m" | |
| "\033[1;92m" + git_head[0].strip() + "\033[0;0m , \033[1;91m" + git_head[1].strip() + "\033[0;0m" |
| print(" \033[1;93mcommit \033[0;0m: ", git_hash) | ||
| print(" \033[1;36mHEAD \033[0;0m: ", git_head) | ||
| print(" \033[1;31mmodified files\033[0;0m (since last commit):") | ||
| print(os.popen('git diff-index --name-only HEAD -- | sed "s/^/ /g"').read()) |
There was a problem hiding this comment.
Using os.popen is discouraged in favor of the subprocess module, which provides better error handling and security. Since subprocess is already imported and used elsewhere in this file, it should be used here as well.
| print(os.popen('git diff-index --name-only HEAD -- | sed "s/^/ /g"').read()) | |
| diff_proc = subprocess.run(["git", "diff-index", "--name-only", "HEAD", "--"], capture_output=True, text=True, check=True) | |
| for line in diff_proc.stdout.splitlines(): | |
| print(" " + line) |
| abs_proj_dir = os.path.join(os.path.dirname(__file__), "..") | ||
| abs_src_dir = os.path.join(abs_proj_dir, "src") |
There was a problem hiding this comment.
The calculation of abs_proj_dir and abs_src_dir is duplicated across multiple scripts. Since shamrock_tool_banner already defines these variables, they should be reused to improve maintainability.
| abs_proj_dir = os.path.join(os.path.dirname(__file__), "..") | |
| abs_src_dir = os.path.join(abs_proj_dir, "src") | |
| abs_proj_dir = shamrock_tool_banner.abs_proj_dir | |
| abs_src_dir = shamrock_tool_banner.abs_src_dir |
References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
| abs_proj_dir = os.path.join(os.path.dirname(__file__), "..") | ||
| abs_src_dir = os.path.join(abs_proj_dir, "src") |
There was a problem hiding this comment.
The calculation of abs_proj_dir and abs_src_dir is duplicated here as well. Reusing the variables from shamrock_tool_banner would improve maintainability.
| abs_proj_dir = os.path.join(os.path.dirname(__file__), "..") | |
| abs_src_dir = os.path.join(abs_proj_dir, "src") | |
| abs_proj_dir = shamrock_tool_banner.abs_proj_dir | |
| abs_src_dir = shamrock_tool_banner.abs_src_dir |
References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
Workflow reportworkflow report corresponding to commit 8d09382 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportNo relevant changes found. You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review. Doxygen diff with
|
No description provided.