Encoding warning use PEP 597 env var PYTHONWARNDEFAULTENCODING
#733
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request focuses on modernizing type hints across multiple files in the Changes
Assessment against linked issues
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
force-pushed
the
encoding-warning-follow-pep597
branch
from
8fc671b to
3e1ffc5
Compare
DanielYang59
force-pushed
the
encoding-warning-follow-pep597
branch
from
e7d9c76 to
2cd39ce
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #733 +/- ##
==========================================
- Coverage 84.19% 84.18% -0.01%
==========================================
Files 27 27
Lines 1670 1669 -1
Branches 314 314
==========================================
- Hits 1406 1405 -1
Misses 202 202
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: 1
🔭 Outside diff range comments (1)
.github/workflows/test.yml (1)
Windows testing should be re-enabled for path handling verification
The review comment is valid. The evidence shows that:
- Windows testing is explicitly commented out in the workflow (
#, windows-latest)- The codebase handles path operations in
monty.osmodule, including path manipulation and directory operations- While the test suite includes path-related tests, they are not being verified on Windows platform
- The path handling code uses OS-agnostic methods (
os.path), but Windows-specific edge cases should still be verifiedRecommendations:
- Uncomment
windows-latestin the test matrix to ensure cross-platform compatibility- Consider adding Windows-specific test cases for path handling edge cases (backslashes, drive letters)
🔗 Analysis chain
Line range hint
9-10: Verify Windows compatibilityWindows testing appears to be intentionally disabled. Since the changes involve path handling in
monty.os, it would be beneficial to verify Windows compatibility.
Let me check the specific Windows-related path handling in the relevant files, particularly
monty.os.
Let me try a different approach to check for Windows-specific handling and tests.
Let me check for test files in a different way and look for any Windows-specific test cases.
Let me check the specific test cases for OS-related functionality and the current workflow configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Windows-specific path handling code rg -l 'os\.path\.sep|ntpath|\\\\|/|os\.name.*nt' src/monty/Length of output: 467
Script:
#!/bin/bash # Check Windows-specific path handling in monty.os rg -A 5 'os\.name.*nt|ntpath|os\.path\.sep' src/monty/os/__init__.py # Also check for any Windows-specific tests fd test_ src/monty/os/ --exec rg -l 'windows|nt|os\.name'Length of output: 130
Script:
#!/bin/bash # Check content of monty.os for Windows handling cat src/monty/os/__init__.py # Look for test files in monty/os directory fd . src/monty/os/ --type f --full-path | grep testLength of output: 1511
Script:
#!/bin/bash # Look for test files in the entire project related to monty.os fd test.*\.py --exec rg -l "os\.(name|path|sep)|windows|nt" {} # Check if there are any existing GitHub workflow files that include Windows rg -l "windows|Windows" .github/workflows/Length of output: 725
Script:
#!/bin/bash # Check the OS-specific test cases cat tests/test_os.py # Check the current workflow configuration for Windows cat .github/workflows/test.ymlLength of output: 4093
🧹 Nitpick comments (5)
src/monty/bisect.py (1)
Line range hint
22-31: Consider enhancing error messagesThe ValueError could be more descriptive by including the value that wasn't found and the tolerance used.
- raise ValueError + raise ValueError(f"Value {x} not found in list{f' within tolerance {atol}' if atol is not None else ''}")src/monty/serialization.py (1)
47-48: Verify docstring format parameter descriptionThe docstring format parameter description uses quotes around the literal values, which might be confusing as it differs from the type hint syntax. Consider updating for consistency.
- fmt ("json" | "yaml" | "mpk"): If specified, the fmt specified would + fmt (Literal["json", "yaml", "mpk"]): If specified, the fmt specified wouldAlso applies to: 108-109
src/monty/shutil.py (1)
79-81: Consider adding type hints for return valuesWhile updating the type hints, consider adding explicit return type annotations to the functions for better type safety.
def compress_file( filepath: str | Path, compression: Literal["gz", "bz2"] = "gz", target_dir: str | Path | None = None, -) -> None: +) -> None: # explicitly document that function doesn't return anythingAlso applies to: 133-135
src/monty/io.py (1)
79-85: Simplify kwargs.get() call and consider adding docstring noteThe implementation correctly follows PEP 597 for encoding warnings. However, there are two suggestions:
- The
kwargs.get()call can be simplified- Consider documenting the
PYTHONWARNDEFAULTENCODINGbehavior in the function's docstring- if ( - "t" in mode - and kwargs.get("encoding", None) is None - and os.getenv("PYTHONWARNDEFAULTENCODING", False) - ): + if ( + "t" in mode + and kwargs.get("encoding") is None + and os.getenv("PYTHONWARNDEFAULTENCODING", False) + ):Also, consider adding this note to the docstring:
Notes: When PYTHONWARNDEFAULTENCODING environment variable is set (PEP 597), a warning will be issued for text mode operations without explicit encoding.🧰 Tools
🪛 Ruff (0.8.2)
83-83: Use
kwargs.get("encoding")instead ofkwargs.get("encoding", None)Replace
kwargs.get("encoding", None)withkwargs.get("encoding")(SIM910)
tests/test_io.py (1)
435-461: LGTM: Well-structured test for PEP 597 encoding warningsThe test properly validates the behavior of encoding warnings with and without the
PYTHONWARNDEFAULTENCODINGenvironment variable. The warning filters are correctly configured to catch specific warning types.Consider adding a comment explaining the test cases to improve maintainability:
def test_warnings(self, extension, monkeypatch): + """Test encoding warnings based on PEP 597: + 1. Verify warning when PYTHONWARNDEFAULTENCODING is set + 2. Verify no warning when PYTHONWARNDEFAULTENCODING is not set + 3. Verify implicit mode warnings + """🧰 Tools
🪛 Ruff (0.8.2)
444-444: Undefined name
EncodingWarning. Consider specifyingrequires-python = ">= 3.10"ortool.ruff.target-version = "py310"in yourpyproject.tomlfile.(F821)
456-456: Undefined name
EncodingWarning. Consider specifyingrequires-python = ">= 3.10"ortool.ruff.target-version = "py310"in yourpyproject.tomlfile.(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/test.yml(1 hunks)src/monty/bisect.py(1 hunks)src/monty/dev.py(2 hunks)src/monty/functools.py(2 hunks)src/monty/io.py(3 hunks)src/monty/os/__init__.py(3 hunks)src/monty/os/path.py(2 hunks)src/monty/serialization.py(4 hunks)src/monty/shutil.py(3 hunks)src/monty/string.py(2 hunks)src/monty/subprocess.py(1 hunks)src/monty/tempfile.py(1 hunks)tests/test_io.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_io.py
444-444: Undefined name EncodingWarning. Consider specifying requires-python = ">= 3.10" or tool.ruff.target-version = "py310" in your pyproject.toml file.
(F821)
456-456: Undefined name EncodingWarning. Consider specifying requires-python = ">= 3.10" or tool.ruff.target-version = "py310" in your pyproject.toml file.
(F821)
src/monty/io.py
83-83: Use kwargs.get("encoding") instead of kwargs.get("encoding", None)
Replace kwargs.get("encoding", None) with kwargs.get("encoding")
(SIM910)
🔇 Additional comments (15)
.github/workflows/test.yml (1)
19-22: LGTM: Variable naming follows GitHub Actions best practices
The change from matrix.python to matrix.python-version aligns with GitHub Actions' conventional naming patterns, improving clarity and maintainability.
src/monty/os/__init__.py (1)
25-25: LGTM: Type hints modernized per PEP 604
The update from Union[str, Path] to str | Path aligns with PEP 604 and Python 3.10+ compatibility requirements. The changes maintain the same type safety while improving readability.
Also applies to: 45-45
src/monty/bisect.py (1)
22-22: LGTM: Type hints modernized per PEP 604
The update from Optional[float] to float | None aligns with PEP 604 and improves code readability while maintaining the same type safety.
src/monty/subprocess.py (1)
64-64: LGTM! Type hint modernization looks good.
The change from Optional[float] to float | None aligns with PEP 604's union type syntax. The implementation remains thread-safe with proper timeout handling.
src/monty/string.py (2)
10-10: LGTM! Good practice with TYPE_CHECKING import.
The import of Any under TYPE_CHECKING condition helps reduce runtime overhead.
37-37: LGTM! Type hint modernization looks good.
The change from Union[str, Iterable[str]] to str | Iterable[str] aligns with PEP 604's union type syntax. The implementation correctly handles both input types with proper type casting.
src/monty/os/path.py (2)
15-15: LGTM! Good practice with TYPE_CHECKING import.
The import of Callable and Literal under TYPE_CHECKING condition helps reduce runtime overhead.
44-46: LGTM! Type hint modernization looks good.
The changes align with PEP 604's union type syntax:
exts:Union[str, list[str]]→str | list[str]exclude_dirs:Optional[str]→str | Noneinclude_dirs:Optional[str]→str | None
The implementation correctly handles all input types with proper wildcard pattern matching.
src/monty/serialization.py (1)
25-25: LGTM: Type hint modernization properly implemented
The update to modern type hint syntax (using | instead of Union) and the introduction of Literal type for fmt parameter improves type safety by restricting the allowed values to specific strings.
Also applies to: 28-33
src/monty/tempfile.py (1)
42-42: LGTM: Type hint updated to modern syntax
The update from Union[str, Path, None] to str | Path | None aligns with PEP 604 while maintaining the same functionality.
src/monty/shutil.py (1)
15-15: LGTM: Type hints consistently updated
The changes properly implement modern type hint syntax across the file:
- Removed unnecessary Optional import
- Updated type hints from
Optional[str | Path]tostr | Path | None - Maintained consistency across similar parameters
Also applies to: 79-79, 133-133
src/monty/dev.py (1)
20-20: LGTM: Type hint modernization
The changes correctly implement PEP 604 union types, replacing Optional[T] with T | None and Union[T1, T2] with T1 | T2. This improves code readability while maintaining the same type safety.
Also applies to: 26-26, 28-28
src/monty/functools.py (1)
16-16: LGTM: Type hint modernization
The changes correctly implement PEP 604 union types, improving code readability while maintaining type safety.
Also applies to: 133-133
src/monty/io.py (1)
22-22: LGTM: Type hint modernization
The changes correctly implement PEP 604 union types, improving code readability while maintaining type safety.
Also applies to: 26-26, 174-174
tests/test_io.py (1)
429-429: LGTM: Warning check properly added
The FutureWarning check for LZW compression is correctly placed within the context manager block.
|
@esoteric-ephemera Is current change looking good to you? i.e. use the Meanwhile I didn't find where for monty/src/monty/serialization.py Lines 60 to 79 in 26acf0b |
|
Could you set the default encoding for |
I believe this is almost what we're doing: Lines 82 to 90 in 26acf0b Except for after this patch, the encoding warning would only be emitted after I personally prefer to give user the option to turn on this warning:
Does this sound good to you? |
DanielYang59
force-pushed
the
encoding-warning-follow-pep597
branch
from
837a6c1 to
8bc0763
Compare
Summary
PYTHONWARNDEFAULTENCODING, to fix What's the correct way to use theMAGMOM's value when using MP API data? materialsproject/pymatgen#4173 (comment)montyis Python 3.10+Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Documentation