Move Partial Serialization out of MSONable
#744
Conversation
WalkthroughThe pull request introduces significant changes to the JSON serialization functionality in the module. A new private method, Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)
✨ Finishing Touches
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 (
|
![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
🧹 Nitpick comments (2)
src/monty/json.py (2)
1052-1058: Improve docstring formatting and clarity.The docstring could be improved by:
- Using consistent formatting (e.g., Google style)
- Adding parameter descriptions
- Adding return value descriptions
- """Used with the save method. Gets the json representation of a class - with the unserializable components sustituted for hash references.""" + """Gets the JSON representation of a class with unserializable components substituted with hash references. + + Args: + obj: Object to serialize + json_kwargs: Keyword arguments to pass to the JSON encoder + + Returns: + tuple: (encoder, encoded, json_kwargs) + - encoder: MontyEncoder instance used for serialization + - encoded: JSON string representation + - json_kwargs: Keyword arguments used for serialization + """
1060-1084: Fix incorrect parameter documentation.The docstring mentions an unused parameter
pickle_kwargs.- """Encode an object that may contain unhashable parts. - - Parameters - ---------- - json_kwargs : dict - Keyword arguments to pass to the serializer. - pickle_kwargs : dict - Keyword arguments to pass to pickle.dump. - - Returns - ------- - str, dict - The json encoding of the class and the name-object map if one is - required, otherwise None. - """ + """Encode an object that may contain unhashable parts. + + Parameters + ---------- + obj : Any + Object to encode + json_kwargs : dict, optional + Keyword arguments to pass to the serializer. + + Returns + ------- + str, dict + The json encoding of the class and the name-object map if one is + required, otherwise None. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/monty/json.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.9)
🔇 Additional comments (3)
src/monty/json.py (3)
358-375: LGTM! Clean refactoring of serialization logic.The refactoring improves code organization by delegating the serialization logic to a dedicated function.
418-420: LGTM! Proper initialization of kwargs.The initialization of kwargs with empty dictionaries prevents potential NoneType errors and follows best practices.
1020-1023: LGTM! Improved error handling.The change to catch specific AttributeError follows Python's best practices for exception handling.
jmmshn
force-pushed
the
jmmshn/partial
branch
from
3f5da81 to
a616389
Compare
jmmshn
changed the title
MSONable
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/monty/json.py (2)
1052-1058: Fix typo in docstring and remove unused return value.
- The docstring has a typo: "sustituted" should be "substituted"
- The function returns
json_kwargswhich is not used by the callerApply this diff to fix the issues:
- """Used with the save method. Gets the json representation of a class - with the unserializable components sustituted for hash references.""" + """Used with the save method. Gets the json representation of a class + with the unserializable components substituted for hash references.""" encoder = MontyEncoder(allow_unserializable_objects=True, **json_kwargs) encoded = encoder.encode(obj) - return encoder, encoded, json_kwargs + return encoder, encoded
1060-1084: Fix incorrect parameter documentation.The docstring mentions
pickle_kwargsparameter but it's not used in the function.Apply this diff to fix the docstring:
- Parameters - ---------- - json_kwargs : dict - Keyword arguments to pass to the serializer. - pickle_kwargs : dict - Keyword arguments to pass to pickle.dump. + Parameters + ---------- + obj : object + The object to encode. + json_kwargs : dict, optional + Keyword arguments to pass to the serializer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/monty/json.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.9)
🔇 Additional comments (2)
src/monty/json.py (2)
358-375: LGTM! Clean refactoring of get_partial_json.The refactoring improves code organization by delegating the complex logic to a dedicated function while maintaining backward compatibility.
418-420: LGTM! Good defensive programming.Initializing kwargs to empty dicts prevents NoneType errors and ensures consistent behavior.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #744 +/- ##
==========================================
+ Coverage 84.75% 84.91% +0.16%
==========================================
Files 27 27
Lines 1653 1651 -2
Branches 315 312 -3
==========================================
+ Hits 1401 1402 +1
+ Misses 192 191 -1
+ Partials 60 58 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/monty/json.py (1)
1053-1059: Fix typo in docstring.The method is well-focused and follows the single responsibility principle. However, there is a typo in the docstring.
- with the unserializable components sustituted for hash references.""" + with the unserializable components substituted for hash references."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/monty/json.py(4 hunks)tests/test_json.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_json.py
1146-1146: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.9)
🔇 Additional comments (3)
src/monty/json.py (3)
358-376: LGTM! Clean refactoring ofget_partial_json.The method has been nicely refactored to delegate its logic to the new
partial_monty_encodefunction while maintaining backward compatibility.
419-421: LGTM! Good defensive programming.The initialization of
json_kwargsandpickle_kwargsto empty dictionaries whenNoneprevents potentialNoneTypeerrors.
1062-1082: LGTM! Well-documented and focused implementation.The function is well-documented with clear docstrings and follows the single responsibility principle. The implementation correctly handles the encoding of objects with unhashable parts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/monty/json.py(5 hunks)tests/test_json.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_json.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/monty/json.py
[warning] 372-374: src/monty/json.py#L372-L374
Added lines #L372 - L374 were not covered by tests
[warning] 498-498: src/monty/json.py#L498
Added line #L498 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: codecov/patch
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.9)
🔇 Additional comments (3)
src/monty/json.py (3)
1110-1117: LGTM! Well-structured helper function.The
_get_partial_jsonfunction is well-designed as a private helper for JSON serialization with hash references.
1119-1140: LGTM! Clear and well-documented public API.The
partial_monty_encodefunction provides a clean interface for encoding objects with unhashable parts, with proper type hints and documentation.
372-374: Add test coverage for critical paths.The following code paths need test coverage:
get_partial_jsonreturn value handling (lines 372-374)- File existence check in
savemethod (line 498)Would you like me to generate test cases for these scenarios? Here's what we should test:
get_partial_jsonwith empty name_object_mapsavewith existing pickle file and strict=TrueAlso applies to: 498-498
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 372-374: src/monty/json.py#L372-L374
Added lines #L372 - L374 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_json.py (2)
1142-1146: Add version field verification.The test verifies class name and value but doesn't check the version field which is a crucial part of MSONable objects.
assert is_m_d["@class"] == "GoodMSONClass" assert is_m_d["a"] == 1 + assert is_m_d["@version"] == TESTS_VERSION assert len(is_m_map) == 0
1161-1172: Clean up test file after use.The test creates a file but doesn't clean it up after the test completes. This could lead to test pollution.
save(mixed, tmp_path / "mixed.json", strict=False) + (tmp_path / "mixed.json").unlink() # Clean up the test file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_json.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.9)
🔇 Additional comments (3)
tests/test_json.py (3)
22-24: LGTM!The new imports are correctly ordered and align with the functionality being tested.
1146-1146: Fix missing assertion.The test coverage is comprehensive, testing both MSONable and non-MSONable objects. However, there is a missing assertion on line 1146.
- len(is_m_map) == 0 + assert len(is_m_map) == 0
1138-1172: LGTM! Comprehensive test coverage.The test method provides thorough coverage of the partial serialization functionality:
- Tests MSONable object serialization
- Tests non-MSONable object serialization with object references
- Tests mixed object serialization
- Tests file operations with proper error handling
jmmshn
force-pushed
the
jmmshn/partial
branch
from
2d85adf to
5a9f6b3
Compare
jmmshn
changed the title
MSONableMSONable
Move Partial Serialization out of MSONable
The mechanism to attempt to serialize objects and pkl the un-serializable parts is useful outside of MSONable.
Pulled the function out of MSONable and added a
savefunction outside as well.This should not change the behavior of existing code but should enable the ability to do the following
Summary by CodeRabbit