Conversation
Would be nice to prevent importing irrelevant stuff.
Summary of ChangesHello @psss, 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 enhances the project's code quality by activating the Ruff linter's unused import detection and subsequently cleaning up all identified redundant imports. This effort streamlines the codebase, making it more readable and reducing potential for dead code or confusion, thereby improving overall maintainability. Highlights
Changelog
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
|
![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 enables the ruff check for unused imports (F401) and removes a large number of unused imports across the codebase. This is a great cleanup that improves code quality and maintainability. My review includes a couple of suggestions to improve the readability of how some imports are handled, but overall the changes are excellent.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| from tmt.utils import GeneralError, Path | ||
| from tmt.utils import ShellScript as ShellScript | ||
| from tmt.utils import retry as retry | ||
| from tmt.utils import GeneralError, Path, ShellScript, retry # noqa: F401 |
There was a problem hiding this comment.
Huh, what was going on here?
| from typing import Any, Optional, Union | ||
|
|
||
| import tmt | ||
| import tmt as tmt |
There was a problem hiding this comment.
Wanted to look into why it was recommending this. My only assumption is that it is
If an import statement is used to re-export a symbol as part of a module's public interface, consider using a "redundant" import alias, which instructs Ruff (and other tools) to respect the re-export, and avoid marking it as unused, as in:
from module import member as member
But that doesn't seem right in these cases.
Incidentally it does suggest a workaround of
Alternatively, you can use all to declare a symbol as part of the module's interface, as in:
# __init__.py import some_module __all__ = ["some_module"]
There was a problem hiding this comment.
Although would like it if we used
from tmt import utilsas ways to make the namespaces a bit shorter when we otherwise use tmt.utils.GeneralError -> utils.GeneralError
There was a problem hiding this comment.
I'd prefer dropping those unused imports, even if they serve as "reimports", which does not seem to be very common in our code. Shortened version - utils.GeneralError - does not look appealing to me, it seems like it can be very puzzling among the rest tmt.foo imports.
GeneralError itself would be acceptable without the namespace entirely, like other common and frequently used names - Command, Plan, etc.
There was a problem hiding this comment.
I'd prefer dropping those unused imports, even if they serve as "reimports", which does not seem to be very common in our code.
Yes, and the key issue is to find why did ruff consider this as a re-import. If we are genuinely re-importing (which I doubt because it would mean we are calling import tmt.utils.tmt, but there are genuine leftover cases like from tmt.utils import Path) then I think patching the usage would make it work.
Shortened version -
utils.GeneralError- does not look appealing to me, it seems like it can be very puzzling among the resttmt.fooimports.
GeneralErroritself would be acceptable without the namespace entirely, like other common and frequently used names -Command,Plan, etc.
We will easily get too swamped with all of the from ... import. I would rather call from tmt.utils import error and error.GeneralError wherever we have clearly defined namespaced packages where we can do that. base.Command, base.Plan do not feel like a bad compromise.
3 participants
Would be nice to prevent importing irrelevant stuff.
Pull Request Checklist