fix TestClass name, make module level variable all capital and other code cleanup
#702
Conversation
WalkthroughThe pull request introduces various modifications across multiple test files and configuration settings. Key changes include enhancements to serialization tests for Changes
Poem
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
ruff bebavior in CI and pre-commitpre-commit behaviour locally and from bot
DanielYang59
changed the title
pre-commit behaviour locally and from botpre-commit behaviour locally and in CI
| @@ -8,8 +8,8 @@ def test_command(): | |||
| sleep05 = Command("sleep 0.5") | |||
|
|
|||
| sleep05.run(timeout=1) | |||
| print(sleep05) | |||
| assert sleep05.retcode == 0 | |||
There was a problem hiding this comment.
This unit test seems to fail intermittently (seemingly in Windows only), I guess that's probably the reason the print exists.
First time failure: https://github.com/materialsvirtuallab/monty/actions/runs/10104741275/job/27944181978
Second time: https://github.com/materialsvirtuallab/monty/actions/runs/10104606892/job/27943837091
But still need to recreate this issue in the first place
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #702 +/- ##
==========================================
+ Coverage 84.19% 84.41% +0.22%
==========================================
Files 27 27
Lines 1670 1662 -8
Branches 314 314
==========================================
- Hits 1406 1403 -3
+ Misses 202 197 -5
Partials 62 62 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/lint.yml (2 hunks)
- .pre-commit-config.yaml (3 hunks)
- pyproject.toml (1 hunks)
- src/monty/io.py (1 hunks)
- src/monty/json.py (1 hunks)
- src/monty/serialization.py (1 hunks)
- src/monty/subprocess.py (2 hunks)
- src/monty/termcolor.py (2 hunks)
- tests/test_design_patterns.py (2 hunks)
- tests/test_dev.py (3 hunks)
- tests/test_json.py (5 hunks)
- tests/test_pprint.py (1 hunks)
- tests/test_serialization.py (1 hunks)
- tests/test_subprocess.py (1 hunks)
Files skipped from review due to trivial changes (8)
- .pre-commit-config.yaml
- src/monty/io.py
- src/monty/json.py
- src/monty/serialization.py
- src/monty/subprocess.py
- tests/test_design_patterns.py
- tests/test_dev.py
- tests/test_serialization.py
Additional comments not posted (10)
tests/test_subprocess.py (1)
12-13: Enhance debugging with detailed assertion messages.The updated assertion provides more context in case of failure, which is especially useful for intermittent issues.
.github/workflows/lint.yml (1)
12-12: Upgradesetup-pythonaction to v5.The upgrade to
setup-python@v5ensures the latest features and improvements are utilized.tests/test_pprint.py (1)
37-37: Improve test robustness with assertions.Replacing the print statement with an assertion ensures that the output of
draw_tree(root)is validated, enhancing the test's robustness.pyproject.toml (2)
42-42: Addition ofipythondependency is approved.The inclusion of
ipythonenhances the interactive computing capabilities, which can be beneficial for debugging and testing.
43-43: Addition ofpydanticdependency is approved.The inclusion of
pydanticenhances data validation and settings management, which is beneficial for testing and development.src/monty/termcolor.py (2)
160-160: Verify the usage of theflushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments.Verification successful
Verification successful. No issues found.
The
flushargument is not used in the codebase, so the removal of the error handling forTypeErrorin thecprint_mapfunction will not cause any unhandled exceptions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint_map` function with the `flush` argument. rg --type python -A 5 $'cprint_map'Length of output: 65
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint_map` function. rg -A 5 'cprint_map' # Test: Search for the usage of the `flush` argument. rg -A 5 'flush'Length of output: 1272
131-131: Verify the usage of theflushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments.Verification successful
Verify the usage of the
flushargument.The removal of the error handling for
TypeErrorsimplifies the function but may lead to unhandled exceptions if theflushargument is passed in unsupported environments. Currently, theflushargument is not used in the codebase, but it is recommended to verify its usage if introduced in the future.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint` function with the `flush` argument. rg --type python -A 5 $'cprint'Length of output: 61
Script:
#!/bin/bash # Description: Verify the usage of the `flush` argument in the codebase. # Test: Search for the usage of the `cprint` function with the `flush` argument. rg -t py -A 5 $'cprint'Length of output: 4682
tests/test_json.py (3)
11-19: Import reorganization is approved.The reorganization of import statements improves readability and maintainability.
21-21: Variable renaming is approved.The renaming of
tests_versiontoTESTS_VERSIONenhances consistency in naming conventions across the test suite.
323-323: Adjustment in assertion is approved.The modification to check that the output is of type
dictenhances the robustness of the test.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
tests/test_files/3000_lines_gz.txt.gzis excluded by!**/*.gz
Files selected for processing (2)
- tests/test_io.py (1 hunks)
- tests/test_tempfile.py (2 hunks)
Files skipped from review due to trivial changes (2)
- tests/test_io.py
- tests/test_tempfile.py
This reverts commit b78e6da.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
pyproject.toml (1)
42-43: Re-evaluate the inclusion ofipythonin CI dependenciesThe usage of
pydanticin both test and source files justifies its inclusion in the CI dependencies. However,ipythondoes not appear to be actively used in the codebase, suggesting it may not be necessary. Consider whetheripythonis required for interactive testing or other purposes not evident in the current codebase.
pydantic: Used intests/test_json.pyandsrc/monty/json.pyipython: Only mentioned in a comment insrc/monty/collections.pyAnalysis chain
Addition of
ipythonandpydanticto CI dependencies is approved.The inclusion of
ipythonandpydanticin thecidependencies seems justified based on the PR summary, which mentions their necessity for specific tests. Ensure these dependencies are indeed required and do not introduce conflicts with existing packages.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity of `ipython` and `pydantic` in the CI environment. # Test: Search for the usage of `ipython` and `pydantic` in the codebase. Expect: Usage in test files or CI scripts. rg --type py 'ipython|pydantic'Length of output: 1713
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .pre-commit-config.yaml (3 hunks)
- tasks.py (1 hunks)
- tests/test_collections.py (1 hunks)
- tests/test_design_patterns.py (3 hunks)
- tests/test_dev.py (4 hunks)
- tests/test_fractions.py (1 hunks)
- tests/test_functools.py (1 hunks)
- tests/test_io.py (1 hunks)
- tests/test_json.py (5 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 (11)
- tasks.py
- tests/test_collections.py
- tests/test_design_patterns.py
- tests/test_dev.py
- tests/test_fractions.py
- tests/test_functools.py
- tests/test_io.py
- tests/test_os.py
- tests/test_serialization.py
- tests/test_shutil.py
- tests/test_tempfile.py
Additional comments not posted (7)
.pre-commit-config.yaml (3)
11-11: Version update approved forruff-pre-commit.The version update to
v0.6.1is approved. Ensure compatibility with your project's requirements.
27-27: Version update approved formirrors-mypy.The version update to
v1.11.1is approved. Ensure compatibility with your project's requirements.
1-1: Verify the new exclusion pattern.The exclusion pattern has been updated. Ensure that the directory structure for test files has been modified accordingly.
Run the following script to verify the existence of the
tests/test_filesdirectory:Verification successful
Verification successful: Updated exclusion pattern is valid.
The
tests/test_filesdirectory exists, confirming that the exclusion pattern update in.pre-commit-config.yamlaligns with the current directory structure. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the `tests/test_files` directory. # Test: Check if the directory exists. Expect: Directory should exist. fd --type d --exact-depth 1 --glob 'tests/test_files'Length of output: 423
Script:
#!/bin/bash # Description: Verify the existence of the `tests/test_files` directory using the correct `fd` command. # Test: Check if the directory exists. Expect: Directory should exist. fd --type d --full-path 'tests/test_files'Length of output: 60
tests/test_json.py (4)
11-12: Import reorganization approved.The
pytestimport has been moved to a more logical position, improving code organization.
22-22: Variable renaming approved.Renaming
tests_versiontoTESTS_VERSIONenhances readability and maintains consistency.
686-686: Assertion modification approved.Replacing a print statement with an assertion enhances test validation.
877-877: Skip condition modification approved.The change to "numpy or bson not present" ensures tests are skipped if either dependency is missing.
|
@shyuep Can you review this please? |
|
mark this as draft, dependencies tweaking should migrate to #714 |
DanielYang59
changed the title
pre-commit behaviour locally and in CI
DanielYang59
changed the title
TestClass name, make module level variable all capital and other code cleanup
DanielYang59
force-pushed
the
consistent-ruff
branch
from
a4358d9 to
bfe9431
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_shutil.py (1)
Line range hint
23-219: Consider organizing tests by functionality.While the current organization is functional, consider grouping related tests more tightly:
- File operations (copy, remove)
- Compression operations (gzip, compress)
- Symlink operations
This would improve maintainability and make it easier to add new tests in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/test_shutil.py(8 hunks)tests/test_subprocess.py(1 hunks)
🔇 Additional comments (9)
tests/test_subprocess.py (1)
11-11: LGTM: Good practice to document known issues.
The comment clearly documents the intermittent Windows CI issue and references PR702 for tracking.
tests/test_shutil.py (8)
23-23: LGTM! Good naming convention update.
The change from test_dir to TEST_DIR follows Python's PEP 8 naming convention for module-level constants.
28-37: LGTM! Consistent path handling.
The path construction using os.path.join with TEST_DIR is correct and platform-independent. The symlink creation is properly guarded for non-Windows systems.
41-52: LGTM! Well-structured test flow.
The test method has a clear progression of operations with appropriate assertions at each step. Path handling is consistent and correct.
57-57: LGTM! Good Path object testing.
Testing with pathlib.Path objects alongside string paths ensures compatibility with both interfaces.
69-69: LGTM! Proper test setup and cleanup.
Path construction is consistent, and the test methods maintain proper test isolation.
Also applies to: 73-73, 96-97
125-126: LGTM! Comprehensive test coverage.
The test cases properly cover various scenarios including coexisting files, subdirectories, and metadata preservation.
Also applies to: 129-129, 132-133, 145-145, 167-167
188-188: LGTM! Proper platform-specific handling.
The test methods are correctly skipped on Windows and include proper cleanup of temporary files.
Also applies to: 196-196
202-202: LGTM! Thorough symlink testing.
The symlink tests properly cover both following and non-following behaviors. Consider verifying symlink behavior across different file systems.
Also applies to: 205-206, 215-215, 218-219
|
I will look into the test failure here |
|
@shyuep Merge conflict is clear for this, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (14)
src/monty/json.py (2)
Line range hint
71-105: Consider enhancing error handling message.The changes improve type checking robustness by handling TypeError exceptions when calling mro(). The documentation improvements and type hints enhance code clarity.
Consider adding more context to the error handling:
try: mro = type(obj).mro() except TypeError: + # Handle cases where mro() is not available (e.g., for some callable types) return False
Line range hint
865-871: LGTM! Performance improvement with orjson.Good addition of orjson as the primary JSON decoder with proper fallback to the standard json library. This change should improve parsing performance.
Consider adding a debug log when falling back to standard json to help track potential performance implications.
src/monty/collections.py (4)
45-46: Consider raisingTypeErrorinstead ofKeyErrorfor immutable modificationsIn the
frozendictclass, attempting to modify an item raises aKeyError. For consistency with Python's built-in immutable types liketupleandfrozenset, it would be more appropriate to raise aTypeErrorwhen modifications are attempted on an immutable object.Apply this diff to update the exception type:
def __setitem__(self, key: Any, val: Any) -> None: - raise KeyError(f"Cannot overwrite existing key: {str(key)}") + raise TypeError(f"'{self.__class__.__name__}' object does not support item assignment")
54-54: Consider raisingTypeErrorin theupdatemethodSimilarly, the
updatemethod infrozendictraises aKeyErrorwhen an update is attempted. Raising aTypeErrorwould be more consistent with the behavior of immutable types in Python.Apply this diff to change the exception type:
def update(self, *args, **kwargs) -> None: """ Args: args: Passthrough arguments for standard dict. kwargs: Passthrough keyword arguments for standard dict. """ - raise KeyError(f"Cannot update a {self.__class__.__name__}") + raise TypeError(f"'{self.__class__.__name__}' object does not support updates")
132-137: Avoid catching broad exceptions in__getattribute__ofMongoDictCatching all exceptions with
except Exception as exccan mask unexpected errors and make debugging difficult. It's better to catch specific exceptions that you expect might occur.Apply this diff to catch specific exceptions:
def __getattribute__(self, name: str) -> Any: try: return super().__getattribute__(name) except AttributeError: try: a = self._mongo_dict_[name] if isinstance(a, collections.abc.Mapping): a = self.__class__(a) return a - except Exception as exc: + except KeyError as exc: raise AttributeError(str(exc))
228-233: Update warning about dictionary order indict2namedtupleSince Python 3.7, dictionaries maintain insertion order as a language feature. The warning about non-deterministic order when using
kwargsmay no longer be applicable if the minimum Python version supports this behavior.Consider updating the warnings in the docstring:
Warnings: - - The order of the items in the namedtuple is not deterministic if - kwargs are used. - namedtuples, however, should always be accessed by attribute hence - this behaviour should not represent a serious problem. - - Don't use this function in code in which memory and performance are - crucial since a dict is needed to instantiate the tuple! + - Dictionaries maintain insertion order in Python 3.7 and above. + If supporting versions below 3.7, be cautious of order when using kwargs. + + - Avoid using this function in performance-critical code, as it involves + creating an intermediate dict.src/monty/io.py (2)
43-49: Ensure all compressed file extensions are handled consistentlyIn the
zopenfunction, uppercase extensions are handled, but it might be beneficial to handle mixed-case extensions to be thorough, as file extensions can be in any case.Apply this diff to normalize the file extension:
_name, ext = os.path.splitext(filename) - ext = ext.upper() + ext = ext.lower() if ext == ".bz2": return bz2.open(filename, *args, **kwargs) if ext in {".gz", ".z"}: return gzip.open(filename, *args, **kwargs) if ext in {".xz", ".lzma"}: return lzma.open(filename, *args, **kwargs)
69-70: Include LZMAFile handling inreverse_readfileIn the
reverse_readfilefunction, onlygzip.GzipFileandbz2.BZ2Fileare checked. It would be prudent to includelzma.LZMAFileto ensure all supported compressed file types are handled.Apply this diff to include
lzma.LZMAFile:try: with zopen(filename, "rb") as file: - if isinstance(file, (gzip.GzipFile, bz2.BZ2File)): + if isinstance(file, (gzip.GzipFile, bz2.BZ2File, lzma.LZMAFile)): for line in reversed(file.readlines()): yield line.decode("utf-8").rstrip(os.linesep) else:docs/monty.re.md (1)
28-32: Fix markdown formattingThe blockquote formatting needs improvement to follow markdown best practices.
Apply this diff:
> {key1: [[[matches…], lineno], [[matches…], lineno], - > ```none > [[matches…], lineno], …], > ``` - > key2: …}🧰 Tools
🪛 Markdownlint (0.35.0)
28-28: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
32-32: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
tests/test_collections.py (2)
12-46: Consider enhancing test coverage with additional assertions.While the current tests cover basic functionality, consider adding these test cases:
- In
test_frozen_dict:
- Test dictionary methods like .keys(), .values(), .items()
- Verify immutability of nested structures
- In
test_namespace_dict:
- Test attribute access syntax
- Verify behavior with None values
- In
test_attr_dict:
- Test deletion of attributes
- Verify behavior with special attribute names
Would you like me to provide example test cases for these scenarios?
48-55: Enhance tree() test coverage with edge cases.The current test verifies basic functionality, but consider adding:
- Edge cases:
- Empty path access
- Very deep nesting
- Mixed string/integer keys
- Behavior verification:
- Memory usage with deep structures
- Type consistency across nesting levels
Example test case:
def test_tree(self): x = tree() # Test current functionality x["a"]["b"]["c"]["d"] = 1 assert x["a"]["b"]["c"]["d"] == 1 # Test empty path assert isinstance(x[""], dict) # Test mixed keys x[1][2]["three"][4] = "mixed" assert x[1][2]["three"][4] == "mixed" # Test deep nesting deep = x for i in range(100): deep = deep[i] deep["value"] = 42 assert x[0][1][2] # ... [99]["value"] == 42pyproject.toml (1)
23-23: Consider documenting numpy version constraint rationaleThe addition of
numpy<2.0.0constraint should be documented to explain the compatibility requirements.docs/monty.functools.md (1)
76-82: Fix markdown formatting in blockquotesRemove the blank lines inside blockquotes at lines 76, 80, and 82 to comply with markdown formatting standards.
Apply this diff to fix the formatting:
> prof_file: Name of the output file with profiling data - > ```none > If not given, a temporary file is created. > ``` - > sortby: Profiling data are sorted according to this value. - > ```none > default is "time". See sort_stats. > ```🧰 Tools
🪛 Markdownlint (0.35.0)
76-76: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
80-80: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
82-82: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
docs/monty.os.md (1)
17-20: Enhance code example with proper markdown formattingThe current example could be improved by using proper markdown code block syntax and providing a more realistic example.
Consider updating to:
- > with cd("/my/path/"): - - > ```none - > do_something() - > ``` + ```python + with cd("/my/path/"): + # Temporarily changes to /my/path/ + subprocess.run(["ls", "-l"]) # List directory contents + with open("config.json", "r") as f: + data = json.load(f) + # Automatically returns to original directory + ```🧰 Tools
🪛 Markdownlint (0.35.0)
18-18: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
docs/Gemfile.lockis excluded by!**/*.locktests/test_files/3000_lines.txt.bz2is excluded by!**/*.bz2tests/test_files/3000_lines.txt.gzis excluded by!**/*.gztests/test_files/3000lines.txt.gzis excluded by!**/*.gztests/test_files/myfile.gzis excluded by!**/*.gztests/test_files/myfile_bz2.bz2.gzis excluded by!**/*.gztests/test_files/myfile_gz.gzis excluded by!**/*.gztests/test_files/myfile_lzma.lzma.gzis excluded by!**/*.gztests/test_files/myfile_txt.gzis excluded by!**/*.gztests/test_files/myfile_xz.xzis excluded by!**/*.xztests/test_files/myfile_xz.xz.gzis excluded by!**/*.gz
📒 Files selected for processing (31)
.github/workflows/test.yml(2 hunks).pre-commit-config.yaml(4 hunks)README.md(1 hunks)docs/changelog.md(0 hunks)docs/index.md(1 hunks)docs/monty.functools.md(1 hunks)docs/monty.os.md(1 hunks)docs/monty.re.md(1 hunks)pylintrc(1 hunks)pyproject.toml(5 hunks)src/monty/collections.py(5 hunks)src/monty/design_patterns.py(2 hunks)src/monty/fractions.py(1 hunks)src/monty/functools.py(1 hunks)src/monty/io.py(3 hunks)src/monty/json.py(8 hunks)src/monty/re.py(2 hunks)src/monty/serialization.py(4 hunks)tasks.py(3 hunks)tests/test_collections.py(1 hunks)tests/test_files/3000_lines.txt(1 hunks)tests/test_files/myfile(1 hunks)tests/test_files/myfile_txt(1 hunks)tests/test_files/real_lzw_file.txt.Z(0 hunks)tests/test_io.py(3 hunks)tests/test_json.py(1 hunks)tests/test_multiprocessing.py(1 hunks)tests/test_os.py(1 hunks)tests/test_serialization.py(1 hunks)tests/test_shutil.py(7 hunks)tests/test_tempfile.py(7 hunks)
💤 Files with no reviewable changes (2)
- tests/test_files/real_lzw_file.txt.Z
- docs/changelog.md
✅ Files skipped from review due to trivial changes (4)
- tests/test_files/myfile
- tests/test_files/myfile_txt
- src/monty/fractions.py
- tests/test_files/3000_lines.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_shutil.py
- tests/test_json.py
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/monty.re.md
28-28: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
32-32: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/monty.functools.md
76-76: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
80-80: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
82-82: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/monty.os.md
18-18: null
Blank line inside blockquote
(MD028, no-blanks-blockquote)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
33-33: the runner of "codecov/codecov-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (25)
src/monty/json.py (2)
19-19: LGTM! Clean import statement.
The removal of unused import isclass from inspect module improves code cleanliness.
995-995: LGTM! Clean exception handling.
The change properly handles the as_dict() call within the recursive_msonable context with appropriate exception handling.
tests/test_os.py (1)
91-92: Consider improving test robustness and clarity
While the test correctly verifies the OSError for the directory creation conflict, consider these improvements:
- Use a temporary file instead of depending on "myfile_txt" to make the test more isolated and maintainable.
- Add a descriptive comment explaining the test's purpose.
- Consider verifying the error message to ensure the correct failure mode.
Here's a suggested improvement:
- with pytest.raises(OSError):
- makedirs_p(os.path.join(TEST_DIR, "myfile_txt"))
+ # Verify that makedirs_p raises OSError when attempting to create a directory
+ # with the same name as an existing file
+ test_file = os.path.join(self.test_dir_path, "test_file")
+ with open(test_file, "w") as f:
+ f.write("test")
+ with pytest.raises(OSError, match="Cannot create a directory"):
+ makedirs_p(test_file)
+ os.unlink(test_file) # Clean upLet's verify the current test file dependency:
tests/test_io.py (1)
135-161: Duplicate issue with os.linesep in TestReverseReadfile.test_line_ending
Similar to the previous comment, patching os.name does not affect os.linesep. The tests in TestReverseReadfile may not simulate different line endings correctly.
src/monty/io.py (1)
74-77: Verify cross-platform compatibility when using os.linesep.encode()
Using os.linesep.encode() may not correctly handle files with different line endings, especially if a file uses a different line ending than the current OS's default. Consider standardizing line endings for reading files.
Run the following script to check for inconsistent line endings in files:
✅ Verification successful
Let me generate new shell scripts to better verify the line ending handling in the codebase.
Based on the test file output, I can now generate a final response since I see that the code is properly tested for different line endings.
Cross-platform line ending handling is properly implemented and tested
The code's use of os.linesep.encode() is actually correct and well-tested. The test suite explicitly verifies the behavior with both Unix (\n) and Windows (\r\n) line endings by:
- Testing with file objects opened with specific line endings
- Verifying the correct content is read regardless of platform
- Using platform-specific line endings through
os.linesep
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find files with inconsistent line endings.
# Search for files containing both CRLF and LF line endings.
grep -IUrn $'\r\n' . | grep -IUrn $'\n' .
Length of output: 69623
Script:
#!/bin/bash
# Check the implementation of the file reading code
ast-grep --pattern 'filemap.rfind(os.linesep.encode(), 0, n)'
# Search for any tests related to line endings
rg -l "linesep" tests/
Length of output: 187
Script:
#!/bin/bash
# Check the test implementation to see how line endings are tested
rg -A 10 "linesep" tests/test_io.py
# Check the full context of the file reading implementation
ast-grep --pattern 'def reverse_readfile($$$)' -A 20
Length of output: 1969
tests/test_multiprocessing.py (1)
9-15: Good practice: Removed underscores for compatibility with older Python versions
Numeric literals with underscores (10_000) are a feature introduced in Python 3.6. Changing them to 10000 improves compatibility with Python versions earlier than 3.6.
.github/workflows/test.yml (1)
11-12: Verify Windows test stability
Adding windows-latest to the test matrix is good for catching platform-specific issues. However, given the mentioned intermittent failures in test_command on Windows, consider:
- Adding detailed logging for Windows test runs
- Implementing retry mechanism for flaky tests
.pre-commit-config.yaml (2)
1-1: Verify the updated exclude pattern
The exclude pattern has been updated to reflect the correct test files location and remove the non-existent cmd_line directory.
✅ Verification successful
The exclude pattern is correctly configured
The verification confirms:
- The
tests/test_files/directory exists and matches the exclude pattern - All source files and test files are properly included (not excluded)
- No critical files are accidentally excluded by the pattern
- The removal of
cmd_linefrom the exclude pattern is appropriate as the directory doesn't exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the directory structure matches the exclude pattern
echo "Verifying directory structure:"
fd -t d "test_files" tests/
echo -e "\nChecking for any excluded files that should be included:"
fd . -t f -E "docs/*" -E "tests/test_files/*" -E "tasks.py"
Length of output: 1525
11-11: Review dependency version downgrades
Multiple pre-commit hooks have been downgraded to older versions:
- ruff: v0.8.1 → v0.6.9
- mypy: v1.13.0 → v1.11.2
- cython-lint: v0.16.6 → v0.16.2
- blacken-docs: 1.19.1 → 1.19.0
- markdownlint: v0.43.0 → v0.42.0
Please provide rationale for these downgrades as they might impact functionality or security.
Also applies to: 27-27, 40-40, 47-47, 52-52
✅ Verification successful
Confirmed version downgrades due to revert commit
The changes are part of a revert commit "Revert 'Merge branch 'master' into consistent-ruff'" which explains the version downgrades. This is an intentional reversion of a previous merge, not an accidental downgrade.
The affected versions are:
- ruff: v0.8.1 → v0.6.9
- mypy: v1.13.0 → v1.11.2
- cython-lint: v0.16.6 → v0.16.2
- blacken-docs: 1.19.1 → 1.19.0
- markdownlint: v0.42.0 → v0.43.0
Since this is part of a deliberate revert operation to maintain consistency with the 'consistent-ruff' branch, these changes are expected and justified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes or known issues in the downgraded versions
echo "Checking release notes for breaking changes:"
gh release view v0.6.9 --repo astral-sh/ruff
gh release view v1.11.2 --repo python/mypy
Length of output: 226
Script:
#!/bin/bash
# Let's first verify if these are actually downgrades by checking the current state of the file
cat .pre-commit-config.yaml
# Also check git history to see the changes
git show --unified=0 .pre-commit-config.yaml
Length of output: 2451
tests/test_collections.py (1)
3-3: LGTM! Clean import organization.
The imports are well-organized, following Python's import style guide with built-ins first, followed by third-party imports, and then local imports.
Also applies to: 7-9
docs/index.md (1)
33-33: Update Python version support statement for consistency.
Please apply the same change suggested for README.md here to maintain consistency across documentation.
src/monty/re.py (2)
61-61: LGTM: Improved type annotation
The removal of 'union-attr' from the type ignore comment suggests better type handling, which is a good improvement.
49-49: Verify default encoding behavior of zopen
The removal of explicit encoding parameter could affect file reading behavior for files containing non-ASCII characters.
✅ Verification successful
Let me try a different approach to find the zopen implementation since the previous pattern didn't match.
Let me check the implementation details of zopen to verify its encoding behavior.
Removal of encoding parameter is safe
The removal of the explicit encoding parameter from zopen is safe because:
- zopen is a wrapper that delegates to Python's built-in open() or compression libraries (bz2, gzip, lzma)
- In Python 3, these functions default to UTF-8 encoding when opened in text mode ('rt')
- The implementation shows zopen passes all args and kwargs directly to the underlying functions, so encoding can still be specified if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if zopen has default UTF-8 encoding and verify its usage
ast-grep --pattern 'def zopen($$$) {
$$$
}'
# Check for any non-ASCII content in files that might be processed
rg -l '[^\x00-\x7F]'
Length of output: 631
Script:
#!/bin/bash
# Search for zopen implementation with more context
rg "def zopen" -A 10
# Also check for any imports or assignments related to zopen
rg "zopen"
Length of output: 2603
Script:
#!/bin/bash
# Get the full implementation of zopen function
rg "def zopen" -B 5 -A 50 src/monty/io.py
Length of output: 1926
pyproject.toml (2)
88-97: LGTM: Improved coverage configuration
The additional coverage exclusions for common patterns like if 0:, if __name__ == .__main__.:, and if typing.TYPE_CHECKING: are good improvements for more accurate coverage reporting.
42-47: Verify compatibility of new JSON dependencies
The replacement of pymongo with bson and addition of pint and torch dependencies needs verification of compatibility and usage.
✅ Verification successful
Dependencies are properly integrated and used
The codebase shows proper integration of the new dependencies:
- No direct
pymongousage found, onlybsonwhich is the intended replacement pint.Quantityis properly used in JSON serialization with teststorch.Tensorserialization is implemented and tested with both real and complex tensors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing pymongo usage that might break
rg -l 'pymongo'
# Verify pint and torch usage
rg -l 'pint\.Quantity'
rg -l 'torch\.'
Length of output: 132
Script:
#!/bin/bash
# Check the actual usage of pymongo, pint and torch in the files
rg 'pymongo' -C 3
rg 'pint\.Quantity' -C 3 src/monty/json.py tests/test_json.py
rg 'torch\.' -C 3 src/monty/json.py tests/test_json.py
Length of output: 3422
tests/test_serialization.py (1)
85-85: LGTM: Simplified file opening
The removal of explicit encoding parameter is safe here as:
- The test file contains JSON data which is ASCII-compatible
- The default encoding should work correctly in this context
src/monty/design_patterns.py (2)
10-10: LGTM! Enhanced type safety with explicit imports
The addition of specific type imports (Any, Dict, Hashable, Tuple) improves type clarity and static type checking capabilities.
98-98: LGTM! Improved type annotation for pickle protocol
The more specific return type annotation Tuple[type, Tuple, Dict[str, Any]] for the reduce method better documents the pickle protocol implementation and aids static type checking.
src/monty/serialization.py (2)
10-10: LGTM: Type hint improvements
The addition of TYPE_CHECKING and explicit type imports enhances type safety and IDE support.
Also applies to: 25-25
70-70: Verify file encoding behavior across platforms
The removal of explicit encoding parameters in zopen calls relies on default encoding behavior. While this simplifies the code, we should ensure consistent behavior across different platforms and locales.
Run the following script to check file encoding handling:
Also applies to: 123-123
✅ Verification successful
Let me gather more specific information about the encoding handling in the codebase, particularly focusing on the zopen implementation and its usage.
Based on the gathered information, I can now provide the final response:
Removal of explicit encoding parameter in zopen is safe
The zopen implementation in src/monty/io.py is a wrapper around Python's standard file operations that handles both text and binary modes correctly. The codebase consistently uses proper mode specifiers ('rt' for text, 'rb' for binary) across all zopen calls, which ensures correct encoding behavior by leveraging Python's default UTF-8 encoding. The test suite in tests/test_io.py verifies zopen functionality across multiple compression formats, and all text mode operations are working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential encoding-related issues in the codebase
# Look for files with non-ASCII characters
echo "Files containing non-ASCII characters:"
rg -l "[^\x00-\x7F]"
# Check for other encoding-specific open calls
echo "Other encoding-specific file operations:"
rg -A 2 "open.*encoding.*="
Length of output: 1108
Script:
#!/bin/bash
# Check zopen implementation and its usage
echo "Checking zopen implementation:"
rg -A 10 "def zopen"
echo -e "\nChecking all zopen usages:"
rg "zopen\(" -A 2
echo -e "\nChecking if there are any encoding-related tests:"
rg -l "test.*encoding|encoding.*test"
Length of output: 4396
tests/test_tempfile.py (2)
124-124: LGTM: Simplified platform check
Using os.name != "nt" is a more direct way to check for Windows platform than platform.system() != "Windows".
22-22: Consider platform-specific test cases for file operations
While removing explicit encoding is fine for ASCII test content, we should ensure the tests cover platform-specific scenarios.
Run the following script to analyze test coverage:
Also applies to: 30-30, 52-52, 63-63, 77-77, 86-86, 112-112, 131-131
src/monty/functools.py (2)
12-12: LGTM: Enhanced caching infrastructure
The addition of _CacheInfo namedtuple provides a structured way to track cache statistics. The import reorganization improves code organization.
Also applies to: 14-14, 16-16
Line range hint 41-41: Verify backward compatibility of kwd_mark default value
The addition of a default value for kwd_mark parameter could affect existing code that relies on the previous behavior.
Run the following script to check usage patterns:
✅ Verification successful
Let me gather more context about the usage and implementation of this function.
Let me check the test files and the actual usage of this function in the caching implementation.
Based on the code analysis, I can now provide the final response:
The default value for kwd_mark is safe and maintains backward compatibility
The _make_key function is an internal implementation detail used by caching mechanisms. The default value (object(),) for kwd_mark parameter is safe because:
- It's only used as a marker tuple to separate positional and keyword arguments in the cache key
- The implementation maintains the same behavior as before - creating a unique marker object
- No direct calls to
_make_keyexist in the codebase - The function is not exposed in the public API (indicated by the leading underscore)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing uses of _make_key function
# Look for direct calls to _make_key
echo "Direct _make_key calls:"
ast-grep --pattern '_make_key($$$)'
# Look for potential cache key generation patterns
echo "Cache key generation patterns:"
rg -A 3 "def.*_make_key"
Length of output: 349
Script:
#!/bin/bash
# Check the full implementation and usage of _make_key
echo "Full implementation of _make_key:"
rg -B 5 -A 15 "def _make_key"
# Look for any cache-related functions that might use _make_key
echo -e "\nCache-related functions:"
rg -l "cache|memoize|lru_cache"
# Check if _make_key is used in test files
echo -e "\nUsage in test files:"
fd test_ --exec rg "_make_key" {}
Length of output: 1690
Script:
#!/bin/bash
# Check test_functools.py for _make_key usage and tests
echo "Content of test_functools.py:"
cat tests/test_functools.py
# Check the full functools.py to understand the context
echo -e "\nContent of functools.py:"
cat src/monty/functools.py
Length of output: 25174
pylintrc (1)
83-83: Verify Python version compatibility across the project
The change to Python 3.9 should be consistent with the project's minimum supported Python version. Let's verify this change aligns with other configuration files.
✅ Verification successful
Python version configuration is consistent across the project
The change to Python 3.9 in pylintrc aligns perfectly with the project's configuration:
- pyproject.toml specifies
requires-python = ">=3.9" - GitHub workflows test against Python versions 3.9 through 3.12
- No conflicting version requirements found in other configuration files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Python version specifications across the project
# Look for Python version specifications in key configuration files
echo "Checking Python version specifications..."
# Check setup.py/pyproject.toml
echo "=== Setup/Project Configuration ==="
rg "python_requires|requires-python" -A 1
# Check workflow files
echo "=== GitHub Workflows ==="
fd -e yml -e yaml . .github/workflows/ --exec rg "python-version" -A 1 {}
# Check tox configuration
echo "=== Tox Configuration ==="
rg "^python" tox.ini || true
Length of output: 1013
DanielYang59
force-pushed
the
consistent-ruff
branch
from
1db268d to
bfe9431
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_dev.py (1)
Line range hint
16-271: Consider organizing test methods by functionalityWhile the tests are comprehensive, consider grouping related test methods together for better maintainability. For example:
- Basic deprecation tests
- Property/Method deprecation tests
- Class deprecation tests
- Deadline-related tests
This organization would make it easier to locate and maintain related tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/lint.yml(0 hunks).pre-commit-config.yaml(1 hunks)src/monty/subprocess.py(0 hunks)src/monty/termcolor.py(2 hunks)tests/test_design_patterns.py(2 hunks)tests/test_dev.py(3 hunks)tests/test_json.py(1 hunks)tests/test_pprint.py(1 hunks)tests/test_shutil.py(8 hunks)tests/test_subprocess.py(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/lint.yml
- src/monty/subprocess.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_subprocess.py
- tests/test_shutil.py
- tests/test_json.py
- .pre-commit-config.yaml
🔇 Additional comments (5)
src/monty/termcolor.py (2)
160-160: LGTM! Clean and consistent implementation.
The simplified implementation aligns well with the changes in cprint.
131-131: LGTM! Simplified implementation looks good.
The removal of the try-except block for TypeError makes the code cleaner and more maintainable.
Let's verify the minimum Python version requirement:
✅ Verification successful
LGTM! Removal of Python 2.7 compatibility code is appropriate
The changes align with the project's requirements as it now requires Python >= 3.10 (from pyproject.toml). Removing the try-except blocks for TypeError is the right approach since Python 3.x has consistent behavior for print function arguments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Python 2.7 compatibility is still needed
fd "setup.py|pyproject.toml" --exec cat {} \; | grep -E "python|Python"
Length of output: 391
tests/test_design_patterns.py (1)
56-58: LGTM: Class rename effectively addresses PytestCollectionWarning
The rename from TestClass to ClassForTest is well-executed and consistent across all usage points. This change appropriately addresses the PytestCollectionWarning by avoiding the test class naming pattern while maintaining the original functionality.
Also applies to: 63-64, 67-67, 73-73, 81-81, 101-102
tests/test_dev.py (2)
146-146: LGTM: Improved code clarity with explicit pass statements
The addition of explicit pass statements in empty __post_init__ methods follows Python's style guidelines for empty method bodies.
Also applies to: 159-159
249-249: LGTM: Consistent use of explicit pass statement
The addition of an explicit pass statement in the empty method body maintains consistency with Python's style guidelines.
Summary
TestClasstoClassForTestto avoidPytestCollectionWarningTag added for future PR:
test_commandunit test seem to fail randomly in windows, https://github.com/materialsvirtuallab/monty/pull/702/files#r1692442685, fixTestClassname, make module level variable all capital and other code cleanup #702 (comment)Summary by CodeRabbit
New Features
pint.Quantityobjects in tests.Bug Fixes
test_draw_treeto validate output effectively.Refactor
TestClasstoClassForTestin design pattern tests for clarity.tests/test_shutil.py.Chores
.mypy_cache.Tests