-
-
Notifications
You must be signed in to change notification settings - Fork 21
clean up unused Python code and pass vulture pre-commit checks #1112
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
base: main
Are you sure you want to change the base?
Conversation
- removed unused variables flagged by vulture - simplified header guard regeneration logic - ensured pre-commit vulture hook runs cleanly
Summary of ChangesHello @BDadmehr0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing code quality and maintaining a clean codebase by addressing unused Python code and refining development workflows. It specifically removes dead code identified by the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully cleans up unused Python code and configures the vulture pre-commit hook. The changes simplify the codebase as intended. However, I've identified a pre-existing critical issue in the header guard regeneration logic in generate_files.py. The simplified code retains a bug that causes file content to be deleted. My review includes a detailed comment on this issue.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request cleans up unused variables in generate_files.py and correctly configures the vulture pre-commit hook to scan the entire project. While the cleanup is good, I've identified a pre-existing critical bug in the header guard regeneration logic that this PR does not address. The current logic will erroneously delete all content within header guards, leading to data loss. I have provided a specific comment and suggestion to fix this critical issue.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a good job of cleaning up unused code and configuring the vulture pre-commit hook. The simplification of the header guard regeneration logic in generate_files.py is a good intention and fixes a bug where file content was being deleted. However, the new logic introduces a different bug related to handling nested preprocessor directives, which could lead to incorrect file generation. I've provided a detailed comment and a suggested fix for this issue.
|
@jbampton pls review this pr thanks |
jbampton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run the Python file ?
|
run and regenerated .h files @jbampton |
BaseMax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of this PR.
|
Just one question, it automatically check to run trim on file content to remove empty whitespace lines from top and bottom of the files? @jbampton What do you think? |
Not exactly — it trims content inside the guard using .strip(), so top/bottom empty lines are mostly removed, and it always ends with a \n. But it doesn’t explicitly clean leading/trailing blank lines before rebuilding the file. |
#1075