Make it better! #50
Make it better! #50
Conversation
WalkthroughThis pull request introduces version 5.2.0 of the persiantools library, featuring several improvements across different components. The changes include updates to character conversion methods using regular expressions, enhancements to the JalaliDate and JalaliDateTime classes with improved code structure and documentation, and expanded test coverage. The version number, changelog, and license have been correspondingly updated to reflect these modifications. Changes
Possibly related PRs
Poem
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 (
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 92.96% 93.44% +0.47%
==========================================
Files 5 5
Lines 938 930 -8
==========================================
- Hits 872 869 -3
+ Misses 66 61 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
persiantools/characters.py (1)
17-18: LGTM! Consider following Python's naming convention for constants.The regex pattern compilation is a good optimization. However, consider using uppercase for the entire constant names (e.g.,
_AR_TO_FA_PATTERN) since these are module-level constants.-AR_TO_FA_PATTERN = re.compile("|".join(map(re.escape, CHARACTER_MAP_AR_TO_FA.keys()))) -FA_TO_AR_PATTERN = re.compile("|".join(map(re.escape, CHARACTER_MAP_FA_TO_AR.keys()))) +_AR_TO_FA_PATTERN = re.compile("|".join(map(re.escape, CHARACTER_MAP_AR_TO_FA.keys()))) +_FA_TO_AR_PATTERN = re.compile("|".join(map(re.escape, CHARACTER_MAP_FA_TO_AR.keys())))tests/test_characters.py (1)
35-37: LGTM! Consider adding a comment explaining the test case.The test case is valuable as it covers multiple character conversions. Consider adding a comment explaining what specific aspects of the conversion this test case validates.
+ # Test multiple character conversions: Arabic Ya (ي), Persian Ya (ی), Arabic Kaf (ك), and diacritics input_string = "يياكشيسِ" expected_output = "ییاکشیس" self.assertEqual(characters.ar_to_fa(input_string), expected_output)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)LICENSE(1 hunks)persiantools/__init__.py(1 hunks)persiantools/characters.py(4 hunks)persiantools/jdatetime.py(9 hunks)tests/test_characters.py(2 hunks)tests/test_jalalidate.py(13 hunks)
✅ Files skipped from review due to trivial changes (3)
- LICENSE
- persiantools/init.py
- CHANGELOG.md
🔇 Additional comments (13)
persiantools/characters.py (2)
41-41: Great performance optimization!The new regex-based implementation is more efficient as it handles all replacements in a single pass through the string, compared to multiple string replacements in the previous version.
64-64: LGTM! Consistent implementation with ar_to_fa.The implementation maintains consistency with the ar_to_fa function, sharing the same efficient regex-based approach.
tests/test_characters.py (2)
27-28: LGTM! Good edge case coverage.Adding explicit None input testing strengthens the test suite's coverage of edge cases.
59-60: LGTM! Consistent error handling test coverage.Good addition of None input testing for fa_to_ar, maintaining consistency with ar_to_fa's error handling tests.
tests/test_jalalidate.py (4)
Line range hint
15-71: Well-structured and comprehensive test cases!The test cases for Jalali to Gregorian conversion are thorough and cover:
- Wide date range (1100-1505)
- Edge cases around year transitions
- Leap year scenarios
Line range hint
93-118: Excellent bidirectional conversion coverage!The test cases for Gregorian to Jalali conversion are well-designed with:
- Historical dates (1827-2119)
- Matching pairs with the Jalali to Gregorian tests
- Edge cases around calendar transitions
Line range hint
291-319: Good coverage of leap year edge cases!The test cases effectively validate non-leap years, particularly focusing on:
- Years that are exceptions to the 33-year leap year rule
- Sequential years to verify boundary conditions
431-431: Good test coverage for week-related calculations!The test cases properly validate:
- Week of year calculation
- Weekday determination
- ISO weekday conversion
Also applies to: 444-445, 450-450
persiantools/jdatetime.py (5)
135-149: Excellent optimization using frozenset!The changes improve the code by:
- Using frozenset for faster membership testing in is_leap()
- Making the collection immutable to prevent accidental modifications
- Adding an explicit MIN_NON_LEAP_CORRECTION constant for better readability
Line range hint
204-217: Clean and concise initialization logic!The initialization has been improved through:
- Better use of tuple unpacking
- Clearer flow of logic
- More maintainable code structure
222-257: Well-documented properties!The added docstrings:
- Follow proper documentation format
- Clearly describe return values
- Improve code maintainability
Line range hint
342-347: Improved readability of leap year validation!The condition check is now:
- More readable with proper parentheses
- Maintains the same logic with better clarity
1032-1077: Excellent property documentation in JalaliDateTime!The added docstrings:
- Follow proper documentation format
- Clearly describe return values
- Improve code maintainability
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_digits.py (1)
88-90: LGTM! Consider adding a descriptive message.Good addition of an edge case test for floating-point number handling. The test case improves coverage and is well-placed with other exception tests.
Consider adding a descriptive message to the assertion to clarify the specific scenario being tested:
with pytest.raises(digits.OutOfRangeException): - digits.to_word(0.123456789012345) + digits.to_word(0.123456789012345, msg="Should raise OutOfRangeException for floating-point numbers with too many decimal places")CHANGELOG.md (1)
4-9: Consider adding more specific details to the changelog entries.While the changelog entries follow the correct format, they could be more specific about the actual changes made. This helps users and developers better understand the impact of the update.
Consider expanding the entries like this:
## [5.2.0](https://github.com/majiidd/persiantools/compare/5.1.1...5.2.0) - 2025-01-17 -Enhanced character conversion functions using regular expressions. -Improved date handling in Jalali date and time classes. -Added new test cases for edge cases and date conversions. +- Enhanced character conversion functions using regular expressions for better performance and maintainability. +- Improved date handling in Jalali date and time classes with better error handling and validation. +- Added new test cases for edge cases in number conversion and date handling, improving overall test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)tests/test_digits.py(1 hunks)tests/test_jalalidate.py(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_jalalidate.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: windows-latest / 3.13
Summary by CodeRabbit
Release Notes for Version 5.2.0
New Features
Improvements
Testing
Maintenance