Declare required and optional dependency #714
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #714 +/- ##
==========================================
+ Coverage 81.02% 82.42% +1.40%
==========================================
Files 27 27
Lines 1586 1582 -4
Branches 361 285 -76
==========================================
+ Hits 1285 1304 +19
+ Misses 236 215 -21
+ Partials 65 63 -2 ☔ View full report in Codecov by Sentry. |
DanielYang59
force-pushed
the
declare-opt-dependency
branch
from
a5fc5c4 to
473dbc4
Compare
DanielYang59
force-pushed
the
declare-opt-dependency
branch
from
5b9a8c8 to
172f796
Compare
DanielYang59
force-pushed
the
declare-opt-dependency
branch
from
172f796 to
fe58998
Compare
DanielYang59
force-pushed
the
declare-opt-dependency
branch
from
91b5dc4 to
200da7e
Compare
WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
DanielYang59
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (25)
tests/test_fractions.py (4)
Line range hint
8-9: Consider adding more test cases fortest_gcdWhile the current test case is valid, it only checks one scenario. To ensure robust testing of the
gcdfunction, consider adding more test cases, such as:
- GCD of two numbers
- GCD of negative numbers
- GCD with zero as one of the inputs
- GCD of large numbers
Line range hint
12-13: Consider expanding test cases fortest_lcmThe current test case is valid but limited. To improve the test coverage for the
lcmfunction, consider adding more test cases, such as:
- LCM of two numbers
- LCM of negative numbers
- LCM with zero as one of the inputs
- LCM of large numbers
- LCM of numbers with common factors
Line range hint
16-18: LGTM: Good test forgcd_float, consider expandingThe
test_gcd_floatfunction is well-implemented:
- It tests floating-point GCD calculation.
- Uses
pytest.approxfor appropriate floating-point comparison.- Includes a case with a small perturbation to test numerical stability.
To further improve the test coverage, consider adding:
- A test case with negative floating-point numbers.
- A test case with very large and very small floating-point numbers to check for potential overflow or underflow issues.
- A test case where the GCD should be a whole number to ensure correct rounding behavior.
Line range hint
1-18: Consider implementing a more comprehensive test suiteWhile the current tests cover basic functionality for
gcd,lcm, andgcd_float, a more comprehensive test suite would ensure robustness across various scenarios. Consider implementing:
- Parameterized tests to cover multiple input combinations efficiently.
- Edge case tests (e.g., very large numbers, zero, negative numbers).
- Property-based tests to verify mathematical properties (e.g., commutativity, associativity).
- Tests for error handling (e.g., invalid inputs).
This would significantly increase confidence in the correctness of these mathematical functions across a wide range of inputs.
Would you like assistance in generating a more comprehensive test suite?
src/monty/multiprocessing.py (2)
12-13: Improved error handling for tqdm import.The new error handling is more descriptive and aligns well with the optional dependency structure. It provides clearer feedback to users about the requirement for
tqdm.Consider adding a comment above the import statement to indicate that
tqdmis an optional dependency. This can help users understand why the import might fail. For example:# tqdm is an optional dependency required for the imap_tqdm function try: from tqdm.autonotebook import tqdm except ImportError as exc: raise ImportError("tqdm must be installed for this function.") from exc
Line range hint
16-41: Enhance type hinting for better static analysis.While the function implementation is solid, we can improve the type hinting to provide better static analysis support.
Consider the following improvements:
- Use
TypeVarfor the iterable and return type to preserve type information:from typing import Callable, Iterable, TypeVar, List T = TypeVar('T') R = TypeVar('R') def imap_tqdm(nprocs: int, func: Callable[[T], R], iterable: Iterable[T], *args, **kwargs) -> List[R]: ...
- Specify the types for
*argsand**kwargs:from typing import Any def imap_tqdm(nprocs: int, func: Callable[[T], R], iterable: Iterable[T], *args: Any, **kwargs: Any) -> List[R]: ...These changes will provide more precise type information, especially when using static type checkers like mypy.
tests/test_collections.py (4)
Line range hint
26-30: LGTM with suggestion: test_attr_dict covers main functionality.The test method effectively demonstrates the dual nature of
AttrDict, showing both attribute and dictionary-style access. It also verifies that changes are reflected correctly.Consider adding a test for dictionary-style assignment to ensure complete coverage:
d['baz'] = 3 assert d.baz == 3
Line range hint
32-43: LGTM with suggestion: test_frozen_attrdict covers key functionality.This test method effectively demonstrates the behavior of
FrozenAttrDict, showing both attribute-style and dictionary-style access, as well as the immutability of the dictionary. The use ofpytest.raisesfor checking various immutability aspects is appropriate.Consider separating the different exception cases for improved clarity and easier debugging:
with pytest.raises(KeyError): d["foo"] = "bar" with pytest.raises(KeyError): d.foo = "bar" with pytest.raises(KeyError): d.hello = "new"
Line range hint
46-53: LGTM with suggestion: test_tree demonstrates nested structure creation and access.The test method effectively shows how the
treefunction creates a nested dictionary-like structure and allows for deep key access. It's good that it checks for both the presence and absence of keys at different levels.Consider adding type checks to ensure each level is of the expected type:
assert isinstance(x, dict) assert isinstance(x['a'], dict) assert isinstance(x['a']['b'], dict) assert isinstance(x['a']['b']['c'], dict)This would provide additional confidence in the
treefunction's behavior.
Line range hint
1-53: Overall: Well-structured and comprehensive test suite for collections module.This test file provides good coverage for the various collection types in the
monty.collectionsmodule. The tests are organized logically and cover both positive and negative cases for each collection type. The use ofpytestfeatures likepytest.raisesis appropriate and consistent throughout the file.A few minor suggestions were made to improve individual tests, but overall, this is a solid foundation for ensuring the reliability of the collections module.
Consider adding parameterized tests for some of the collection types to test with a wider range of inputs and edge cases. This could help catch any potential issues with different data types or structures.
tests/test_os.py (1)
Line range hint
38-47: Remove duplicate test method in TestCd class.The
TestCdclass contains two identical test methods:test_cdandtest_cd_exception. Having duplicate tests doesn't provide any additional coverage and may lead to confusion. Consider removing thetest_cd_exceptionmethod to keep the test suite clean and efficient.Suggested change:
class TestCd: def test_cd(self): with cd(TEST_DIR): assert os.path.exists("empty_file.txt") assert not os.path.exists("empty_file.txt") - def test_cd_exception(self): - with cd(TEST_DIR): - assert os.path.exists("empty_file.txt") - assert not os.path.exists("empty_file.txt")pyproject.toml (2)
22-23: LGTM! Consider specifying a minimum version for ruamel.yaml.The addition of "ruamel.yaml" and the version constraint for numpy are good improvements. They align with the PR objectives and ensure better dependency management.
Consider specifying a minimum version for ruamel.yaml to ensure compatibility:
- "ruamel.yaml", + "ruamel.yaml>=0.17.21",
36-53: LGTM! Well-organized optional dependencies. Consider adding a comment for the 'json' group.The new optional dependency groups are well-organized and align perfectly with the PR objectives. This structure will make it easier for users to install only the dependencies they need.
Consider adding a brief comment for the 'json' group, similar to the one for 'dev', to clarify its purpose:
+ # json is for extended JSON functionality json = [ "bson", "orjson", "pandas", "pydantic", "pint", "torch", ]tests/test_serialization.py (1)
Line range hint
23-27: LGTM: Improved test cleanup process.The
teardown_classmethod has been updated to useglobfor finding and removing test files. This change enhances the cleanup process by catching files with various extensions, reducing the risk of leftover files affecting subsequent test runs.Consider adding a print statement or logging to indicate when files are being cleaned up, which could be helpful for debugging:
for fn in files_to_clean_up: print(f"Cleaning up test file: {fn}") os.remove(fn)tasks.py (3)
Line range hint
137-143: LGTM with a suggestion for improvement.The new
set_verfunction is well-implemented, using context managers for file operations and explicitly setting the encoding. However, the regex pattern for version matching could be more specific to avoid potential false matches.Consider using a more specific regex pattern to ensure only the correct version line is matched:
contents = re.sub(r'^\s*version\s*=\s*["\']([\d.]+)["\']', f'version = "{version}"', contents, flags=re.MULTILINE)This pattern will only match lines that start with optional whitespace, followed by "version", optional whitespace, an equals sign, optional whitespace, and then the version number in quotes.
Line range hint
146-156: LGTM with a suggestion for improvement.The modifications to the
releasefunction, including the newversionparameter and the call toset_ver, improve the flexibility and consistency of the release process. The order of operations is logical and comprehensive.Consider adding version string validation to prevent potential issues with invalid version strings:
import re def is_valid_version(version: str) -> bool: return bool(re.match(r'^\d+\.\d+\.\d+$', version)) @task def release(ctx: Context, notest: bool = False, version: str = NEW_VER) -> None: if not is_valid_version(version): raise ValueError(f"Invalid version string: {version}") set_ver(ctx, version) # ... rest of the functionThis ensures that only valid semantic version strings (e.g., "1.2.3") are accepted.
Line range hint
102-105: Consider deprecating or updatingsetverfor consistency.With the introduction of
set_verwhich updatespyproject.toml, thesetverfunction might be redundant or lead to inconsistencies.Consider one of the following options:
- Deprecate
setverifsetup.pyis no longer needed for version management.- Update
setverto use the same approach asset_ver:@task def setver(ctx: Context, version: str = NEW_VER) -> None: with open("setup.py", "r", encoding="utf-8") as f: contents = f.read() contents = re.sub(r'version=["\']([\d.]+)["\']', f'version="{version}"', contents) with open("setup.py", "w", encoding="utf-8") as f: f.write(contents)This ensures consistency in version management across files and uses a more robust Python-based approach.
src/monty/serialization.py (3)
12-12: Approve the direct import of YAML and suggest removing redundant checksThe direct import of
YAMLfromruamel.yamlaligns with the PR objective of declaring it as a required dependency. This simplifies the import logic and makes the code more straightforward.Consider removing the redundant checks for
YAML is Nonein theloadfnanddumpfnfunctions, as these checks are no longer necessary. The import will raise an error ifruamel.yamlis not installed. Here's a suggested change for theloadfnfunction (apply similar changes todumpfn):if fmt == "yaml": - if YAML is None: - raise RuntimeError("Loading of YAML files requires ruamel.yaml.") yaml = YAML() return yaml.load(fp, *args, **kwargs)This change will make the code more concise and align it with the new dependency management approach.
Line range hint
29-85: Consider removing redundant YAML error handling inloadfnThe
loadfnfunction's logic is correct and aligns with the file's purpose. However, with the direct import ofYAMLfromruamel.yaml, the error handling for missingYAMLin the function body is now redundant.Consider removing the redundant check and error raising for
YAML is None:if fmt == "yaml": - if YAML is None: - raise RuntimeError("Loading of YAML files requires ruamel.yaml.") yaml = YAML() return yaml.load(fp, *args, **kwargs)This change will make the code more concise and align it with the new dependency management approach, while maintaining the function's core functionality.
Line range hint
88-144: Consider removing redundant YAML error handling indumpfnThe
dumpfnfunction's logic is correct and aligns with the file's purpose. However, similar to theloadfnfunction, the error handling for missingYAMLin the function body is now redundant due to the direct import ofYAMLfromruamel.yaml.Consider removing the redundant check and error raising for
YAML is None:if fmt == "yaml": - if YAML is None: - raise RuntimeError("Loading of YAML files requires ruamel.yaml.") yaml = YAML() yaml.dump(obj, fp, *args, **kwargs)This change will make the code more concise and align it with the new dependency management approach, while maintaining the function's core functionality.
tests/test_io.py (1)
Line range hint
199-213: Great addition: TestFileLock classThe new
TestFileLockclass is a valuable addition to the test suite. It covers theFileLockfunctionality, which wasn't tested before. The use of pytest fixtures for setup and teardown is a good practice.One suggestion for improvement:
Consider using awithstatement in thetest_raisemethod to ensure the new lock is always released, even if an exception other thanFileLockExceptionis raised:def test_raise(self): with pytest.raises(FileLockException): with FileLock(self.file_name, timeout=1) as new_lock: new_lock.acquire()This change would make the test more robust and prevent potential issues with unreleased locks in case of unexpected errors.
src/monty/io.py (2)
Line range hint
63-89: Consider usingcontextlib.closingfor file handling inreverse_readfile.To ensure proper resource management, consider using
contextlib.closingfor file handling in thereverse_readfilefunction. This can help prevent resource leaks in case of exceptions.Import
closingfromcontextlibat the beginning of the file:from contextlib import closingThen, modify the
reverse_readfilefunction as follows:def reverse_readfile(filename: Union[str, Path]) -> Generator[str, str, None]: try: with closing(zopen(filename, "rb")) as file: if isinstance(file, (gzip.GzipFile, bz2.BZ2File)): for line in reversed(file.readlines()): yield line.decode("utf-8").rstrip(os.linesep) else: with mmap.mmap(file.fileno(), 0, access=mmap.ACCESS_READ) as filemap: n = len(filemap) while n > 0: i = filemap.rfind(os.linesep.encode(), 0, n) yield filemap[i + 1 : n].decode("utf-8").rstrip(os.linesep) n = i except ValueError: returnThis change ensures that the file and mmap objects are properly closed, even if an exception occurs.
Line range hint
220-307: Consider usingcontextlib.contextmanagerforFileLock.The
FileLockclass can be simplified by using the@contextlib.contextmanagerdecorator. This can make the code more concise and easier to maintain.Here's a suggested refactoring of the
FileLockclass:from contextlib import contextmanager @contextmanager def file_lock(file_name: str, timeout: float = 10, delay: float = 0.05): lock = FileLock(file_name, timeout, delay) try: lock.acquire() yield lock finally: lock.release() class FileLock: def __init__(self, file_name: str, timeout: float = 10, delay: float = 0.05) -> None: self.file_name = os.path.abspath(file_name) self.lockfile = f"{self.file_name}.lock" self.timeout = timeout self.delay = delay self.is_locked = False self.fd = None if self.delay > self.timeout or self.delay <= 0 or self.timeout <= 0: raise ValueError("delay and timeout must be positive with delay <= timeout") def acquire(self) -> None: start_time = time.time() while True: try: self.fd = os.open(self.lockfile, os.O_CREAT | os.O_EXCL | os.O_RDWR) self.is_locked = True break except OSError as exc: if exc.errno != errno.EEXIST: raise if (time.time() - start_time) >= self.timeout: raise FileLockException(f"{self.lockfile}: Timeout occurred.") time.sleep(self.delay) def release(self) -> None: if self.is_locked: os.close(self.fd) os.unlink(self.lockfile) self.is_locked = False self.fd = NoneThis refactoring simplifies the
FileLockclass and provides a more Pythonic way to use it with the context manager.src/monty/json.py (1)
Line range hint
931-948: Added support for numpy arrays and generic types in jsanitizeThe addition of numpy array and generic type handling in the
jsanitizefunction is a valuable improvement. It ensures that these types are properly sanitized for JSON serialization.However, there's a potential optimization for the numpy array handling:
Consider using numpy's
tolist()method directly for non-object dtypes to improve performance. Here's a suggested modification:if isinstance(obj, np.ndarray): + if obj.dtype.kind in 'biufc': + return obj.tolist() try: return [ jsanitize( i, strict=strict, allow_bson=allow_bson, enum_values=enum_values, recursive_msonable=recursive_msonable, ) for i in obj.tolist() ] except TypeError: return obj.tolist()This change would avoid the list comprehension for numeric dtypes, potentially improving performance for large arrays.
tests/test_dev.py (1)
Line range hint
96-97: Remove@pytest.fixture()decorator from test methodThe
@pytest.fixture()decorator is intended for fixture functions that provide setup for tests, not for actual test methods. Decoratingtest_deprecated_deadline_no_warnwith@pytest.fixture()may prevent it from being recognized and executed as a test by pytest. Additionally, includingselfas a parameter is not appropriate for fixtures.Apply this diff to correct the test method:
- @pytest.fixture() def test_deprecated_deadline_no_warn(self, monkeypatch): """Test cases where no warning should be raised."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- pyproject.toml (2 hunks)
- src/monty/dev.py (1 hunks)
- src/monty/io.py (1 hunks)
- src/monty/itertools.py (1 hunks)
- src/monty/json.py (6 hunks)
- src/monty/multiprocessing.py (1 hunks)
- src/monty/serialization.py (1 hunks)
- tasks.py (1 hunks)
- tests/test_collections.py (1 hunks)
- tests/test_design_patterns.py (1 hunks)
- tests/test_dev.py (1 hunks)
- tests/test_fractions.py (1 hunks)
- tests/test_functools.py (1 hunks)
- tests/test_io.py (1 hunks)
- tests/test_json.py (3 hunks)
- tests/test_os.py (1 hunks)
- tests/test_serialization.py (1 hunks)
- tests/test_shutil.py (1 hunks)
- tests/test_tempfile.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/monty/dev.py
🧰 Additional context used
🔇 Additional comments (37)
tests/test_fractions.py (1)
4-4: LGTM: Improved code formattingThe addition of a blank line between import statements and function imports enhances code readability and adheres to PEP 8 style guidelines.
src/monty/multiprocessing.py (1)
Line range hint
1-41: Overall assessment: Good changes with room for minor improvements.The changes to error handling for the
tqdmimport are beneficial, providing clearer feedback to users. Theimap_tqdmfunction remains well-implemented and documented. Consider the suggested improvements to type hinting and adding a comment about the optional dependency to further enhance the code quality and maintainability.tests/test_collections.py (3)
Line range hint
1-8: LGTM: Import statements are appropriate and complete.The import statements are well-organized and include all necessary modules and classes for the tests. The use of
__future__import is a good practice for ensuring compatibility.
Line range hint
13-17: LGTM: test_frozen_dict covers basic functionality.The test method effectively checks both the immutability of
frozendict(by attempting to add a new key) and the correct retrieval of existing values. Good use ofpytest.raisesfor exception testing.
Line range hint
19-24: LGTM: test_namespace_dict covers key functionality.This test method effectively demonstrates the behavior of the
Namespaceclass, showing that new keys can be added but existing ones cannot be updated. The use ofpytest.raisesfor checking the immutability of existing keys is appropriate.tests/test_os.py (4)
Line range hint
1-9: LGTM: Import statements are well-organized.The import statements are correctly structured, with built-in modules first, followed by third-party imports (pytest), and then local imports from the monty package. The added empty line (line 7) provides good separation between import groups.
Line range hint
15-35: LGTM: TestPath class provides good test coverage.The TestPath class contains well-structured tests for the
zpathandfind_extsfunctions:
test_zpathcorrectly verifies the behavior for both gzipped and non-gzipped files.test_find_extsprovides comprehensive coverage, including tests for:
- Counting Python files
- Counting bz2 files
- Excluding specific directories
- Including specific directories
These tests align well with the PR objectives and ensure the robustness of the os-related functions.
Line range hint
50-63: LGTM: TestMakedirs_p class is well-structured and comprehensive.The
TestMakedirs_pclass provides a thorough test suite for themakedirs_pfunction:
- The
setup_methodcorrectly initializes the test directory path.test_makedirs_pcovers multiple scenarios:
- Successful directory creation
- Re-creation of an existing directory without errors
- Attempting to create a file instead of a directory (expecting an OSError)
- The
teardown_methodproperly cleans up the created directory after tests.The use of
pytest.raisesfor checking theOSErroris appropriate and aligns with best practices for exception testing.
Line range hint
1-63: Overall, the test file is well-structured and comprehensive.The
tests/test_os.pyfile provides excellent coverage for the os-related functions in the monty package. The tests are well-organized into separate classes, each focusing on specific functionalities. They align well with the PR objectives of enhancing dependency management and streamlining imports.Key strengths:
- Comprehensive testing of
zpath,find_exts,cd, andmakedirs_pfunctions.- Good use of pytest fixtures and assertions.
- Proper setup and teardown methods where necessary.
Areas for improvement:
- Remove the duplicate test method in the
TestCdclass, as mentioned in a previous comment.Consider adding more edge cases or error scenarios if applicable, but overall, this test file provides a solid foundation for ensuring the reliability of the os-related functions.
pyproject.toml (3)
29-34: LGTM! Good updates to CI dependencies.The updates to pytest and pytest-cov versions, along with the addition of type stubs for orjson and requests, are excellent improvements. These changes will enhance the development and testing processes.
111-111: LGTM! Good addition to isort configuration.Adding "monty" to the known-first-party list for isort is a good practice. This will help maintain consistent and clean import statements throughout the project.
Line range hint
1-111: Excellent improvements to dependency management!The changes in this file significantly enhance the project's dependency management:
- Required dependencies are clearly defined.
- Optional dependencies are well-organized by module.
- CI dependencies are updated with specific versions.
- The new structure provides flexibility for users to install only needed dependencies.
These improvements align perfectly with the PR objectives and will make the project easier to maintain and use.
tests/test_serialization.py (2)
10-11: LGTM: Import statements reorganized.The import statements for
dumpfn,loadfn, andScratchDirhave been moved to the top of the file. This change improves code organization and adheres to PEP 8 style guidelines.
Line range hint
1-94: Overall assessment: Good improvements to test organization and cleanup.The changes made to this file enhance code organization and test cleanup without altering the core functionality of the tests. The import statements have been properly reorganized, and the teardown process has been improved. These changes align well with the PR objectives of enhancing dependency management and improving code structure.
src/monty/itertools.py (3)
Line range hint
15-114: LGTM: Functions remain unchanged and compatible with the new import structure.The functions
chunks,iterator_from_slice,iuptri, andilotrihave not been modified. They continue to use numpy correctly with the new import structure. This maintains the existing functionality while simplifying the overall code structure.
Line range hint
1-114: Summary: Import simplification aligns with PR objectivesThe changes in this file successfully simplify the import structure by directly importing numpy. This aligns well with the PR objective of making numpy a required dependency. The existing functions remain unchanged, maintaining their functionality while benefiting from the simplified import structure.
Overall, these changes improve code clarity without introducing new risks. Ensure that the verification step for the numpy dependency in pyproject.toml is completed to maintain consistency between the code and the project configuration.
10-10: LGTM: Direct import of numpy simplifies the code.The removal of the conditional import for numpy and its direct import at the top of the file aligns with the PR objective of making numpy a required dependency. This change simplifies the code and makes the dependency more explicit.
To ensure that this change is properly reflected in the project configuration, please run the following script:
✅ Verification successful
Verified: numpy is correctly listed as a dependency in pyproject.toml and the import changes are appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify numpy is listed as a required dependency in pyproject.toml # Test: Check if numpy is listed in pyproject.toml if grep -q 'numpy.*"' pyproject.toml; then echo "numpy is correctly listed as a dependency in pyproject.toml" else echo "Warning: numpy is not listed as a dependency in pyproject.toml" fiLength of output: 164
src/monty/serialization.py (1)
Line range hint
1-144: Summary of changes and suggestionsThe main change in this file is the direct import of
YAMLfromruamel.yaml, which aligns with the PR objectives of declaring it as a required dependency. This change simplifies the import logic and makes the code more straightforward.To further improve the code:
- Remove the redundant checks for
YAML is Nonein bothloadfnanddumpfnfunctions.- Consider updating the error messages to reflect that
ruamel.yamlis now a required dependency.These changes will make the code more concise and align it with the new dependency management approach while maintaining the core functionality of the serialization module.
tests/test_tempfile.py (2)
7-7: Improved import organizationThe addition of a blank line between standard library imports and project-specific imports enhances code readability and adheres to PEP 8 style guidelines. This small change contributes to better code organization.
Line range hint
1-164: Verify test coverage for potentialScratchDirchangesWhile this test file remains largely unchanged, it's important to ensure that any modifications to the
ScratchDirclass resulting from the dependency management changes in this PR are adequately covered by these tests. Please verify if any new functionality or edge cases need to be added to maintain comprehensive test coverage.To help verify the test coverage, you can run the following command to check for any recent changes to the
ScratchDirclass:This script will help identify if there have been any recent changes to the
ScratchDirclass that might require additional test coverage.tests/test_io.py (5)
8-8: LGTM: Improved import organizationThe addition of an empty line between import groups improves readability and adheres to PEP 8 guidelines for import organization.
Line range hint
67-73: Great addition: Test for empty file handlingThe new
test_empty_filemethod is a valuable addition to the test suite. It ensures that thereverse_readlinefunction handles empty files correctly, which is an important edge case. This test improves the overall robustness of the code and aligns well with the PR objectives.
Line range hint
141-147: Excellent: Consistent empty file test for reverse_readfileThe addition of
test_empty_fileforreverse_readfileis a great improvement. It ensures consistent behavior withreverse_readlinewhen handling empty files. This test enhances the robustness of the codebase and maintains parity between related functions.
Line range hint
191-196: Excellent addition: Testing Path object compatibilityThe new
test_Path_objectsmethod is a valuable addition to theTestZopenclass. It ensures thatzopencan handlePathobjects from thepathlibmodule, which is the recommended way of working with file paths in modern Python. This test improves the robustness of thezopenfunction and aligns well with current best practices.
Line range hint
1-213: Overall excellent improvements to the test suiteThe changes made to
tests/test_io.pysignificantly enhance the test coverage and robustness of the codebase. Key improvements include:
- Testing empty file handling for both
reverse_readlineandreverse_readfile.- Ensuring compatibility with
pathlib.Pathobjects inzopen.- Adding comprehensive tests for the
FileLockfunctionality.These additions align well with the PR objectives of improving dependency management and enhancing functionality. They cover important edge cases and modern Python features, which will contribute to the overall stability and maintainability of the project.
Great work on improving the test suite!
src/monty/io.py (1)
Line range hint
309-328: Consider platform-independent implementation forget_open_fds.The
get_open_fdsfunction currently only works on UNIX-like systems. Consider implementing a platform-independent version or providing alternatives for other operating systems.Here's a suggestion for a more platform-independent implementation:
import os import psutil def get_open_fds() -> int: """ Get the number of open file descriptors for the current process. Returns: int: The number of open file descriptors for the current process. """ try: return psutil.Process().num_fds() except AttributeError: # Fallback for systems where num_fds() is not available return len(psutil.Process().open_files())This implementation uses the
psutillibrary, which provides cross-platform system and process utilities. You'll need to addpsutilto your project dependencies.Let's check if
psutilis already a dependency:#!/bin/bash # Check if psutil is already a dependency grep -q "psutil" pyproject.toml && echo "psutil is already a dependency" || echo "psutil needs to be added as a dependency"tests/test_json.py (4)
11-12: LGTM: Import changes improve code clarity and align with project goals.The direct imports of
numpyandpytest, along with the renaming of__version__toTESTS_VERSION, simplify the code and follow better naming conventions. These changes are in line with the PR objective of makingnumpya required dependency.Also applies to: 23-23
320-320: LGTM: Consistent use of renamed constant.The use of
TESTS_VERSIONinstead oftests_versionis consistent with the import change and follows the convention of using uppercase for constants.
871-871: LGTM: Minor adjustment in test assertion.The change in the assertion statement doesn't significantly alter the test's functionality and appears to be a minor refinement.
Line range hint
1-1134: Overall, changes improve code quality and align with PR objectives.The modifications in this file, including import changes, variable renaming, and minor adjustments, contribute to better code clarity and consistency. These changes align well with the PR's objective of enhancing dependency management. No significant issues or concerns were identified during the review.
src/monty/json.py (3)
594-607: Improved numpy array handling in MontyEncoderThe changes to the numpy array handling in the
defaultmethod ofMontyEncoderare well-implemented. The new code:
- Removes the unnecessary check for
npbeing None, as numpy is now a required dependency.- Handles both real and complex numpy arrays more consistently.
- Simplifies the overall structure of the array encoding.
These improvements make the code more robust and easier to maintain.
608-609: Simplified numpy generic type handlingThe handling of numpy generic types has been streamlined by removing the unnecessary check for
npbeing None. This change is consistent with the numpy array handling improvements and makes the code more concise.
Line range hint
807-815: Improved numpy array decoding in MontyDecoderThe changes to numpy array handling in the
MontyDecoderclass are well-implemented and consistent with the improvements made in theMontyEncoderclass. The code now handles both real and complex numpy arrays efficiently, without unnecessary checks.tests/test_design_patterns.py (1)
9-9: Importingpytestfor test assertionsThe addition of
import pytestis necessary for utilizingpytestfunctionalities such aspytest.raisesin your test cases.tests/test_shutil.py (1)
12-12: Appropriate addition ofimport pytestThe inclusion of
import pytestis necessary for usingpytestfeatures such aspytest.raisesandpytest.warnsin the test cases.tests/test_dev.py (1)
10-10: Added import forpytestmoduleThe
pytestmodule is now imported to support the use ofpytest.raises,pytest.warns, and other pytest functionalities in the test cases.tests/test_functools.py (1)
8-8: Appropriate addition ofimport pytestThe addition of
import pytestis necessary for usingpytestfeatures such aspytest.raisesin the test cases below. This ensures that the tests correctly handle and assert expected exceptions.
pyproject.toml
Outdated
| @@ -30,7 +30,6 @@ ci = [ | |||
| "monty[optional]", | |||
| "pytest>=8", | |||
| "pytest-cov>=4", | |||
| "types-orjson", | |||
There was a problem hiding this comment.
This should be unnecessary now:
Note: The orjson package includes type annotations or type stubs since version 3.6.1. Please uninstall the types-orjson package if you use this or a newer version.
orjson 3.6.1 is released Aug 4, 2021
Update: pinned lower bound in 3edd4d5 just in case (it's recommended to pin a lower bound to avoid outdated version for any dependency anyway)
|
@shyuep Please review this, thank you! |
This reverts commit b89d7a9.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/test.yml (1)
8-8: Approved: Addition offail-fast: falseenhances test coverage.The addition of
fail-fast: falseto the strategy is a good change. It allows all jobs in the matrix to continue running even if one fails, which can help identify issues across different environments in a single run.However, be aware that this might lead to longer overall run times and potentially higher costs, especially if there are persistent failures in specific environments.
Consider monitoring the workflow run times and costs after this change. If they increase significantly, you might want to implement conditional logic to abort the workflow early in case of multiple failures.
pyproject.toml (2)
35-36: LGTM: New dev dependency group addedThe addition of a specific dev group for the "dev" module is a good organizational practice. The comment helps clarify its purpose.
Consider updating the comment to be more specific:
-# dev is for "dev" module, not for development +# 'dev' group is for the "dev" module, not for development dependenciesThis minor change could further reduce any potential confusion.
41-52: LGTM: Well-organized optional dependency groupsThe addition of specific optional dependency groups (json, multiprocessing, serialization, task) and the reorganization of the 'optional' group are excellent improvements. This structure aligns perfectly with the PR objectives and will simplify the installation process for users.
For consistency, consider capitalizing the first letter of each group name in the comments, similar to how you've done for the 'dev' group:
-# dev is for "dev" module, not for development +# Dev is for "dev" module, not for development +# Json is for json-related optional dependencies +# Multiprocessing is for multiprocessing-related optional dependencies +# Serialization is for serialization-related optional dependencies +# Task is for task-related optional dependenciesThis minor change would improve readability and maintain consistency across the file.
src/monty/dev.py (1)
234-235: Add test coverage for new error handling.The static analysis tool indicates that these new lines are not covered by tests. To ensure the robustness of this change and prevent potential regressions, it's important to add a test case that covers this scenario.
Consider adding a test case that attempts to call
install_excepthookin an environment where IPython is not available, and verify that it raises the expected ImportError with the correct message.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 234-235: src/monty/dev.py#L234-L235
Added lines #L234 - L235 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/test.yml (1 hunks)
- pyproject.toml (2 hunks)
- src/monty/dev.py (1 hunks)
- src/monty/io.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/monty/io.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/monty/dev.py
[warning] 234-235: src/monty/dev.py#L234-L235
Added lines #L234 - L235 were not covered by tests
🔇 Additional comments (5)
pyproject.toml (4)
22-23: LGTM: Dependencies updated as per PR objectivesThe addition of "ruamel.yaml" as a required dependency and the version constraint on numpy (<2.0.0) are in line with the PR objectives and represent good dependency management practices.
29-33: LGTM: CI dependencies updated appropriatelyThe updates to the ci optional dependencies, including the addition of coverage tools and newer versions of pytest, are appropriate for maintaining robust testing and code quality practices.
110-110: LGTM: Ruff configuration updated for better import sortingThe addition of
lint.isort.known-first-party = ["monty"]to the ruff configuration is a good practice. This will help maintain clean and consistent import statements throughout the project by correctly categorizing imports from the monty package as first-party imports.
Line range hint
1-110: Summary: Excellent improvements to dependency managementOverall, the changes to the pyproject.toml file are well-structured and align perfectly with the PR objectives. The updates improve dependency management by:
- Adding required dependencies (ruamel.yaml and numpy) with appropriate version constraints.
- Reorganizing optional dependencies into logical groups.
- Updating CI dependencies for better testing and coverage.
- Improving import sorting configuration.
These changes will significantly enhance the project's dependency management and make it easier for users to install only the dependencies they need. The minor suggestions provided in the review comments are for further refinement and consistency, but the core changes are solid and beneficial to the project.
src/monty/dev.py (1)
234-235: Improved error handling for missing IPython dependency.The change from issuing a warning to raising an ImportError is a good improvement. It provides a clearer and more actionable message when the IPython dependency is missing. This aligns well with the PR's objective of enhancing dependency management.
However, there are two points to consider:
- The function's docstring should be updated to reflect that it now may raise an ImportError.
- A test should be added to cover this new error handling.
To ensure the docstring is updated, please run:
Would you like assistance in updating the docstring and creating a test for this new error handling?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 234-235: src/monty/dev.py#L234-L235
Added lines #L234 - L235 were not covered by tests
Summary
rualmel.yamlandnumpyto required dependencies (as they are used by more than one module), and remove import guard forruamel.yamlandnumpylzmais built in after python 3.3It would then be much easier to install extra dependencies (
pip install monty[json]orpip install monty[optional]to install all extras) without need to dig into the code to find out what is required