-
Notifications
You must be signed in to change notification settings - Fork 1
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
28 is leap year #47
28 is leap year #47
Conversation
- Implemented is_leap function to determine leap years based on divisibility rules. - Added unit tests covering standard cases, edge cases, and defensive tests for invalid input types.
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.
Nice solution! I appreciate how compact it is, although some code norms might consider it less readable. However, I think it works well. The tests pass all the checklist requirements and are thorough and well-rounded. Great job! I have only highlighted a few small changes.
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.
Great job! They passed all the ci checks, and everything seems correct for me. But could you change Created on XX XX XX to specific date?
I'll make these corrections soon, thanks for the feedback 🙌 |
Thank you both @YonatanBest and @Mohamed-Elnageeb. I made all the suggested changes and corrections 🙌. |
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.
They passed all the ci checks. And They are well written. Great Job!
Thank you 🙌 |
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.
I see no changes needed. Good job!
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.
I see no changes needed. Good job!
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.
Well done @Khusro-S , just a few suggestions
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.
The code is well-structured with good test coverage, but there are a few areas for improvement:
Error Handling: Replace AssertionError with more specific exceptions like TypeError for invalid types and ValueError for non-positive years.
Test Coverage: Consider adding a test for the year 0 to raise a ValueError, as it's an invalid input.
Yeah, you’re right, that’s a good point and definitely best practice. I’ll make those changes in a bit. Thank you 🎉 |
… function. - Replaced assert statements with specific error handling (TypeError and ValueError). - Updated test cases to check for the new errors: -- TypeError for non-integer inputs (string, None, float). -- ValueError for negative year inputs.
b627775
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.
Good job @Khusro-S
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.
Great job!
name: solution review
about: A template PR for code review with a checklist
Behavior
Files
/tests/test_file_name.py
Unit Tests
Function Docstring
Raises:
The Function
Strategy
Do's
Don'ts
Implementation
when it's too restricting.