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

Solve py312 warning on pkgutil.get_loader #7597

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

berland
Copy link
Contributor

@berland berland commented Apr 8, 2024

Using importlib instead

Issue

tests/unit_tests/shared/share/test_shell.py::test_shell_script_jobs_names
  /Users/HAVB/venv/3.12-arm64/lib/python3.12/site-packages/ert/config/ert_config.py:71: DeprecationWarning: 'pkgutil.get_loader' is deprecated and slated for removal in Python 3.14; use importlib.util.find_spec() instead
    ert_shared_loader = cast("FileLoader", pkgutil.get_loader("ert.shared"))

Approach
Use importlib.util.find_spec

  • 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 berland self-assigned this Apr 8, 2024
@berland berland added release-notes:skip If there should be no mention of this in release notes maintenance Not a bug now but could be one day, repaying technical debt labels Apr 8, 2024
@berland berland force-pushed the solve_get_loader_warning branch from b31e675 to ec9caf0 Compare April 8, 2024 13:50
@berland berland marked this pull request as ready for review April 8, 2024 13:50
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.22%. Comparing base (b4eedfc) to head (ec9caf0).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7597      +/-   ##
==========================================
+ Coverage   84.51%   85.22%   +0.71%     
==========================================
  Files         388      388              
  Lines       23352    23355       +3     
  Branches      892      892              
==========================================
+ Hits        19735    19904     +169     
+ Misses       3510     3344     -166     
  Partials      107      107              

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

@@ -59,7 +58,7 @@
from .workflow_job import ErtScriptLoadFailure, WorkflowJob

if TYPE_CHECKING:
from importlib.abc import FileLoader
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing TYPE_CHECKING, can we just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ay yes, should have. Sorry I clicked the merge button too soon.

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! 🚢

@berland berland merged commit b4dc96d into equinor:main Apr 9, 2024
37 checks passed
@berland berland mentioned this pull request Apr 9, 2024
9 tasks
@berland berland deleted the solve_get_loader_warning branch May 29, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Not a bug now but could be one day, repaying technical debt 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.

3 participants