Conversation
Signed-off-by: kunalsz <kunalavengers@gmail.com>
Summary of ChangesHello, 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 significantly refactors the language coverage modules by extracting and centralizing common CLI-related functionalities into a new shared module. This change aims to reduce code duplication across different language-specific tools, making the codebase more modular, easier to maintain, and more straightforward to extend when integrating new languages. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good refactoring that deduplicates common CLI logic for language coverage scripts into floss/language/cli_common.py. This improves maintainability and makes it easier to add support for new languages. My feedback focuses on improving the new configure_logging function to correctly handle the --quiet flag, which was previously defined but not used, and making the logging setup more robust.
| def configure_logging(debug): | ||
| level = logging.DEBUG if debug else logging.INFO | ||
| logging.basicConfig(level=level) | ||
| logging.getLogger().setLevel(level) |
There was a problem hiding this comment.
The configure_logging function doesn't handle the --quiet argument. It should set the logging level to CRITICAL when quiet is true to suppress all but fatal errors. Additionally, the call to logging.getLogger().setLevel(level) is redundant because logging.basicConfig() already configures the root logger's level. I've updated the function to handle this, removed the redundant call, and added type hints for clarity.
| def configure_logging(debug): | |
| level = logging.DEBUG if debug else logging.INFO | |
| logging.basicConfig(level=level) | |
| logging.getLogger().setLevel(level) | |
| def configure_logging(debug: bool, quiet: bool): | |
| level = logging.CRITICAL if quiet else (logging.DEBUG if debug else logging.INFO) | |
| logging.basicConfig(level=level) | |
| else: | ||
| logging.basicConfig(level=logging.INFO) | ||
| logging.getLogger().setLevel(logging.INFO) | ||
| configure_logging(debug=args.debug) |
There was a problem hiding this comment.
To correctly handle the --quiet command-line flag, you need to pass args.quiet to the updated configure_logging function.
| configure_logging(debug=args.debug) | |
| configure_logging(debug=args.debug, quiet=args.quiet) |
| pe = pefile.PE(args.path) | ||
| except pefile.PEFormatError as err: | ||
| logger.debug(f"NOT a valid PE file: {err}") | ||
| configure_logging(debug=args.debug) |
There was a problem hiding this comment.
To correctly handle the --quiet command-line flag, you need to pass args.quiet to the updated configure_logging function.
| configure_logging(debug=args.debug) | |
| configure_logging(debug=args.debug, quiet=args.quiet) |
|
Hi @mr-tz , if you are available can you review this 🙃 |
|
@kunalsz this code style is not consistent with the rest of the codebase. the code and message read like AI slop. please go back and demonstrate more effort working with the project or we'll close out this PR. |
Signed-off-by: kunalsz <kunalavengers@gmail.com>
|
Hi @williballenthin, sorry if it came across that way. I made a few improvements, but the logger messages were taken directly from the original code before the refactor |
| @@ -0,0 +1,58 @@ | |||
| # Copyright 2023 Google LLC | |||
There was a problem hiding this comment.
why does this say 2023?
There was a problem hiding this comment.
I used that from the other file, I'll update that to 2026
| logging.getLogger().setLevel(logging.INFO) | ||
|
|
||
|
|
||
| def open_pe_or_none(path: str): |
There was a problem hiding this comment.
i don't think this function is worth it. let's remove.
Signed-off-by: kunalsz <kunalavengers@gmail.com>
2 participants
Resolves #1213
Introduces
language/cli_common.py, deduplicating the argument parsing , logging setup, and PE validation logic into one file.Makes it easier for setting up new languages