Skip to content
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

Implement pylint import-outside-toplevel rule (C0415) #5180

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

ashanbrown
Copy link
Contributor

@ashanbrown ashanbrown commented Jun 19, 2023

Summary

Implements pylint C0415 (import-outside-toplevel) — imports should be at the top level of a file.

The great debate I had on this implementation is whether "top-level" is one word or two (toplevel or top_level). I opted for 2 because that seemed to be how it is used in the codebase but the rule string itself uses one-word "toplevel." 🤷 I'd be happy to change it as desired.

I suppose this could be auto-fixed by moving the import to the top-level, but it seems likely that the author's intent was to actually import this dynamically, so I view the main point of this rule is to force some sort of explanation, and auto-fixing might be annoying.

For reference, this is what "pylint" reports:

> pylint crates/ruff/resources/test/fixtures/pylint/import_outside_top_level.py
************* Module import_outside_top_level
...
crates/ruff/resources/test/fixtures/pylint/import_outside_top_level.py:4:4: C0415: Import outside toplevel (string) (import-outside-toplevel)

ruff would now report:

import_outside_top_level.py:4:5: PLC0415 `import` should be used only at the top level of a file
  |
3 | def import_outside_top_level():
4 |     import string # [import-outside-toplevel]
  |     ^^^^^^^^^^^^^ PLC0415
  |

Contributes to #970.

Test Plan

Snapshot test.

@ashanbrown ashanbrown changed the title Add pylint import-outside-toplevel rule Implement pylint import-outside-toplevel rule (C0415) Jun 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2023

PR Check Results

Ecosystem

ℹ️ ecosystem check detected linter changes. (+10 -0 violations, +0 -0 fixes in 41 projects)

rotki/rotki (+10 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero

+ rotkehlchen/assets/resolver.py:127:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:128:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:158:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:159:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:70:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:71:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:96:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/assets/resolver.py:97:9: PLC0415 `import` should be at the top-level of a file
+ rotkehlchen/tests/conftest.py:133:9: PLC0415 `import` should be at the top-level of a file
+ tools/pyinstaller_hooks/pre_find_module_path/hook-distutils.py:40:5: PLC0415 `import` should be at the top-level of a file

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLC0415 10 10 0 0 0

@ashanbrown ashanbrown marked this pull request as draft June 19, 2023 16:51
@ashanbrown ashanbrown marked this pull request as ready for review June 19, 2023 19:04

/// C0415
pub(crate) fn import_outside_top_level(checker: &mut Checker, stmt: &Stmt) {
if !checker.semantic().at_top_level() && !checker.semantic().in_type_checking_block() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, Pylint only flags this for imports outside of the module scope:

if True:
  import sys  # Pylint doesn't flag this.

def foo():
  import sys  # It does flag this!

I think this should perhaps be !checker.scope().kind.is_module()?

@ashanbrown
Copy link
Contributor Author

@charliermarsh I've gone with your suggestion of just checking the semantic scope kind. I had misinterpreted one of the tests from pylint and it does look like they considered nested conditionals to be at the top-level (and that's why pylint needs no special handling for type-checking).

I've also updated the documentation for this check as I don't think PEP8 completely offers a rationale for this rule. The pylint PR for this rule also doesn't provide a lot of explanation, so I've done my best to offer a more complete rationale for this check, but I'd welcome suggestions.

@ashanbrown
Copy link
Contributor Author

For the record, this is why ChatGPT thinks we should place our imports at the top-level of a module:

why should I place my imports at the top-level of a module in python?
ChatGPT
In Python, it is generally recommended to place your imports at the top-level of a module. Here are some reasons why this is a good practice:

Readability: Placing imports at the top of your module makes it easier for other developers (including yourself) to understand and navigate your code. When imports are at the top, it provides a clear and concise view of the dependencies required by your module.

Avoiding Confusion: Placing imports at the top-level helps prevent potential conflicts and confusion. It allows you to easily see which external modules or libraries your code relies on, reducing the chances of accidentally redefining or overriding names used in other parts of your code.

Consistency: Placing imports at the top-level is a widely followed convention in the Python community. By adhering to this convention, you make your code more familiar and consistent with other Python projects, which can facilitate collaboration and code maintenance.

Dependency Analysis: When imports are located at the top, it becomes easier to analyze the dependencies of a module. Tools like static analyzers or linters can quickly scan the imports and identify any potential issues or unused imports.

Performance: Import statements at the top-level are executed only once when the module is first imported, and subsequent imports of the same module do not execute those statements again. Placing imports within functions or conditional blocks may lead to repeated imports, which can impact performance.

However, it's worth noting that there are a few exceptions or alternative approaches to consider. For instance, if an import is only needed within a specific function or block of code, it can be placed there to improve the performance of the module as a whole. Additionally, there may be cases where conditional imports are required based on runtime conditions. But as a general guideline, placing imports at the top-level is considered a good practice in Python.

@charliermarsh charliermarsh self-requested a review June 25, 2023 22:42
@sbrugman
Copy link
Contributor

The autofix for this could be #1251

@charliermarsh charliermarsh self-assigned this Jul 27, 2023
@charliermarsh charliermarsh force-pushed the import-outside-toplevel branch 2 times, most recently from 33c5da3 to 8f891cf Compare October 29, 2023 16:36
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Oct 29, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) October 29, 2023 16:36
@charliermarsh charliermarsh merged commit 9b89bf7 into astral-sh:main Oct 29, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants