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

Avoid import outside top-level #8295

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Conversation

berland
Copy link
Contributor

@berland berland commented Jul 3, 2024

Keeping as exception at one spot due to circular dependency and one spot due to measurable time-loss to do top-level import.

Ignoring the issue in script/build, this file is being deleted soon.

Issue
Resolves ruff PLC0415

Approach
fix/add exception/comment.

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure tests pass locally (after every commit!)

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@berland
Copy link
Contributor Author

berland commented Jul 3, 2024

import-outside-top-level (PLC0415)

Derived from the Pylint linter.

This rule is in preview and is not stable. The --preview flag is required for use.

What it does

Checks for import statements outside of a module's top-level scope, such
as within a function or class definition.

Why is this bad?

PEP 8 recommends placing imports not only at the top-level of a module,
but at the very top of the file, "just after any module comments and
docstrings, and before module globals and constants."

import statements have effects that are global in scope; defining them at
the top level has a number of benefits. For example, it makes it easier to
identify the dependencies of a module, and ensures that any invalid imports
are caught regardless of whether a specific function is called or class is
instantiated.

An import statement would typically be placed within a function only to
avoid a circular dependency, to defer a costly module load, or to avoid
loading a dependency altogether in a certain runtime environment.

Example

def print_python_version():
    import platform

    print(python.python_version())

Use instead:

import platform


def print_python_version():
    print(python.python_version())

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.88%. Comparing base (cf02f45) to head (a490506).

Files Patch % Lines
src/ert/__main__.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8295   +/-   ##
=======================================
  Coverage   90.88%   90.88%           
=======================================
  Files         347      347           
  Lines       21076    21076           
=======================================
+ Hits        19154    19155    +1     
+ Misses       1922     1921    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@berland berland self-assigned this Jul 3, 2024
@berland berland added the release-notes:skip If there should be no mention of this in release notes label Jul 3, 2024
Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -6,6 +6,10 @@

from ert import LibresFacade
from ert.config import CancelPluginException, ErtPlugin
from ert.gui.ertwidgets.customdialog import CustomDialog
from ert.gui.ertwidgets.listeditbox import ListEditBox
from ert.gui.ertwidgets.models.path_model import PathModel
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I moved these in at some point and it was done for the same reason as

def run_gui_wrapper(args: Namespace, ert_plugin_manager: ErtPluginManager) -> None:
    # Importing ert.gui on-demand saves ~0.5 seconds off `from ert import __main__`
    from ert.gui.main import run_gui  # noqa: PLC0415

When you run ert test_run, gui will not be imported when you place these imports inside getArguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Keeping as exception at one spot due to circular dependency and
one spot due to measurable time-loss to do top-level import.
@berland berland enabled auto-merge (rebase) August 5, 2024 08:34
@berland berland merged commit e831d2a into equinor:main Aug 5, 2024
37 checks passed
@berland berland deleted the ruff-top-level-import branch August 23, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:skip If there should be no mention of this in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants