-
Notifications
You must be signed in to change notification settings - Fork 48
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
Remove import_resources try-except block #323
Conversation
These weird imports were only relevant for Python versions that are past their end of life: 3.6 and older.
Here's the code health analysis summary for commits Analysis Summary
|
This is another rather mechanical change, nothing special. I'll YOLO-merge Wednesday, June 12 unless approved earlier. |
Reviewer's Guide by SourceryThis pull request removes the try-except block for importing 'importlib_resources' and 'importlib.resources' in various test files. The try-except block was previously used to support Python versions 3.6 and older, which are now past their end of life. The changes ensure that only 'importlib.resources' is imported directly. File-Level Changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tovrstra - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 20 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -18,17 +18,14 @@ | |||
# -- | |||
"""Test iodata.formats.chgcar module.""" | |||
|
|||
from importlib.resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add test for import error handling
Consider adding a test case to ensure that the import error handling is no longer necessary. This can help verify that the codebase is fully compatible with the current Python versions.
from importlib.resources import as_file, files | |
import unittest | |
from importlib.resources import as_file, files | |
class TestImportLibResources(unittest.TestCase): | |
def test_imports(self): | |
try: | |
from importlib.resources import as_file, files | |
except ImportError: | |
self.fail("importlib.resources import failed") |
@@ -18,17 +18,14 @@ | |||
# -- | |||
"""Test iodata.formats.extxyz module.""" | |||
|
|||
from importlib.resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Check for redundant imports
Since the import statement is now simplified, ensure that there are no redundant imports in the test files. This will help keep the code clean and maintainable.
from importlib.resources import as_file, files | |
from importlib.resources import as_file |
@@ -18,15 +18,12 @@ | |||
# -- | |||
"""Test iodata.formats.log module.""" | |||
|
|||
from importlib.resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add test for importlib.resources
Consider adding a test case specifically for the importlib.resources module to ensure it behaves as expected in different environments.
@@ -18,16 +18,13 @@ | |||
# -- | |||
"""Test iodata.formats.locpot module.""" | |||
|
|||
from importlib.resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Check for redundant imports
Since the import statement is now simplified, ensure that there are no redundant imports in the test files. This will help keep the code clean and maintainable.
from importlib.resources import as_file, files | |
from importlib.resources import as_file |
@@ -22,17 +22,13 @@ | |||
import os | |||
import subprocess | |||
import sys | |||
from importlib.resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add test for importlib.resources
Consider adding a test case specifically for the importlib.resources module to ensure it behaves as expected in different environments.
@@ -29,11 +30,6 @@ | |||
from ..basis import MolecularBasis, Shell, convert_conventions | |||
from ..overlap import OVERLAP_CONVENTIONS, compute_overlap, factorial2 | |||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Remove redundant try-except block
The try-except block for importlib_resources is no longer necessary. Ensure that the test environment is updated to reflect this change.
try: | |
from importlib_resources import as_file, files |
@@ -27,11 +28,6 @@ | |||
from ..api import dump_many, dump_one, load_many, load_one | |||
from ..utils import FileFormatWarning, angstrom | |||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Remove redundant try-except block
The try-except block for importlib_resources is no longer necessary. Ensure that the test environment is updated to reflect this change.
try: | |
from importlib_resources import as_file, files |
|
||
import numpy as np | ||
from numpy.testing import assert_allclose, assert_equal | ||
|
||
from ..api import dump_one, load_one | ||
from ..utils import angstrom, volume | ||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Remove redundant try-except block
The try-except block for importlib_resources is no longer necessary. Ensure that the test environment is updated to reflect this change.
try: | |
from importlib_resources import as_file, files |
import numpy as np | ||
from numpy.testing import assert_allclose, assert_equal | ||
|
||
from ..api import load_one | ||
from ..formats.qchemlog import load_qchemlog_low | ||
from ..utils import LineIterator, angstrom, kjmol | ||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Remove redundant try-except block
The try-except block for importlib_resources is no longer necessary. Ensure that the test environment is updated to reflect this change.
try: | |
from importlib_resources import as_file, files |
@@ -27,11 +28,6 @@ | |||
from ..utils import FileFormatError, angstrom | |||
from .common import truncated_file | |||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Remove redundant try-except block
The try-except block for importlib_resources is no longer necessary. Ensure that the test environment is updated to reflect this change.
try: | |
from importlib_resources import as_file, files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me
Thanks for checking. |
Another small item on the list of #313
These weird imports were only relevant for Python versions that are past their end of life: 3.6 and older.
Summary by Sourcery
This pull request removes the try-except blocks used for importing 'importlib_resources' to support older Python versions (3.6 and older). The code now directly imports from 'importlib.resources', simplifying the import statements and removing compatibility code for unsupported Python versions.