fix: skip datatype registration warning for duplicate types #856
Conversation
CodSpeed Performance ReportMerging #856 will not alter performanceComparing Summary
|
📝 WalkthroughWalkthroughThe changes introduce Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant DataType
participant System
Tester->>DataType: Create instance (dt1)
Tester->>DataType: Create instance (dt2)
Tester->>DataType: Compare dt1 == dt2 (__eq__)
Tester->>DataType: Print representation (__repr__)
Tester->>System: register_data_type(dt1)
Tester->>System: register_data_type(dt2)
alt dt1 and dt2 are identical
System-->>Tester: No warning
else dt1 and dt2 differ
System-->>Tester: Emit UserWarning
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
dpdata/system.py (1)
1106-1110: Add stacklevel parameter to warnings.warn call.The static analysis tool correctly identifies that the
warnings.warncall should include astacklevelparameter to point to the correct location in the call stack where the warning originates.- if dt != dtypes_dict[dt.name]: - warnings.warn( - f"Data type {dt.name} is registered twice with different definitions; only the newly registered one will be used.", - UserWarning, - ) + if dt != dtypes_dict[dt.name]: + warnings.warn( + f"Data type {dt.name} is registered twice with different definitions; only the newly registered one will be used.", + UserWarning, + stacklevel=2, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dpdata/data_type.py(1 hunks)dpdata/system.py(1 hunks)tests/test_custom_data_type.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
dpdata/data_type.py
[error] 168-168: dpdata.data_type.DataError: test_diff not found in data. This error occurs in multiple test cases during data validation checks in dpdata system initialization.
dpdata/system.py
[error] 221-221: Data check failed in dpdata.system.System.check_data() due to missing 'test_diff' data type in the dataset.
tests/test_custom_data_type.py
[error] 129-129: Unit tests failed due to dpdata.data_type.DataError: test_diff not found in data during setup of TestDeepmdLoadDumpCompUnlabeled tests.
🪛 Ruff (0.12.2)
dpdata/system.py
1107-1107: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🔇 Additional comments (4)
dpdata/data_type.py (2)
67-88: LGTM! Well-implemented equality comparison.The
__eq__method correctly implements equality comparison by checking the instance type and comparing all relevant attributes. The implementation follows Python best practices by returningFalsefor non-DataType objects and comparing all fields that define the identity of a DataType.
90-102: LGTM! Clear and informative string representation.The
__repr__method provides a comprehensive string representation that includes all relevant attributes. The format is clear and follows Python conventions for repr methods.dpdata/system.py (1)
1106-1111: LGTM! Improved warning logic prevents unnecessary alerts.The updated logic correctly uses the new
__eq__method to only warn when data types with the same name have different definitions. This addresses the PR objective of skipping warnings for identical duplicate registrations.tests/test_custom_data_type.py (1)
13-35: LGTM! Comprehensive tests for DataType methods.The test methods for
__eq__and__repr__are well-structured and cover the expected behaviors thoroughly, including edge cases like comparing with non-DataType objects.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #856 +/- ##
=======================================
Coverage 85.41% 85.41%
=======================================
Files 82 82
Lines 7567 7577 +10
=======================================
+ Hits 6463 6472 +9
- Misses 1104 1105 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Chun Cai <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpdata/system.py (1)
1105-1110: Good improvement to the duplicate registration logic!The condition now correctly checks for both name collision AND different definitions before warning, which aligns perfectly with the PR objective. However, consider adding
stacklevel=2to the warning for better debugging experience.if dt.name in dtypes_dict and dt != dtypes_dict[dt.name]: warnings.warn( f"Data type {dt.name} is registered twice with different definitions; only the newly registered one will be used.", UserWarning, + stacklevel=2, )Regarding the past review comment about merging lines 1105-1106: The current multi-line format is actually more readable given the complexity of the condition and warning message, so the suggestion may not be applicable here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/system.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
dpdata/system.py
1106-1106: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: benchmark
Currently dpdata throws a warning if an existed data type is registered again, even if with the same definition. This PR adds a check to skip if the registered data types are identicle.
Summary by CodeRabbit
New Features
Bug Fixes
Tests