-
-
Notifications
You must be signed in to change notification settings - Fork 566
Introduce HolidayBase methods for proper serialization #2333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary by CodeRabbit
WalkthroughThe changes involve restructuring the translation initialization in the Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 300000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2333 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 203 204 +1
Lines 12402 12569 +167
Branches 1777 1797 +20
==========================================
+ Hits 12402 12569 +167 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/holiday_base.py(4 hunks)tests/test_holiday_base.py(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Test build on windows-latest
🔇 Additional comments (3)
tests/test_holiday_base.py (1)
870-879: Good implementation of pickle serialization test for localized entities.The test properly verifies that holiday entities with different language settings can be correctly serialized and deserialized while maintaining their state.
holidays/holiday_base.py (2)
737-765: Well-structured method for translation initialization.Good refactoring to extract the translation initialization logic from
__init__into a dedicated method. This improves code organization and supports the serialization process.
681-684: Good implementation of state restoration.The method correctly restores the object's state and reinitializes the translation function, completing the serialization cycle.
arkid15r
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KJhellico paying off my technical debts 😄? I appreciate it. A lot!
Thank you!
Co-authored-by: Arkadii Yakovets <[email protected]> Signed-off-by: ~Jhellico <[email protected]>
|
arkid15r
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
PPsyrius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🛠️



Proposed change
Introduce
HolidayBase::__getstate__andHolidayBase::__setstate__methods for proper serialization of holidays objects.Resolve #1236.
Type of change
holidaysfunctionality in general)Checklist
make check, all checks and tests are green