-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Add new exceptions in Yasumi. #95
Conversation
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.
change author in phpdoc class
@stelgenhof you prefer me in author or you ? |
@qneyrat You can put yourself as an author in the Class comments section :) and leave the file comments as is. |
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 changes themselves are looking good, but can you explain why you would like these to be added?
To catch specific exception and not invalidArgument is so generic. |
@stelgenhof are you ready to merge ? |
@qneyrat I believe there are some unanswered questions in the review of your PR. |
@stelgenhof which ones ? |
@rmasclef There is a small conversation regarding the use of the To me it looks like it is not concluded :) |
@stelgenhof indeed, I agree with that one 👍 |
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.
Please also update the CHANGELOG.md file describing the changes in this PR.
@@ -7,7 +7,7 @@ | |||
* For the full copyright and license information, please view the LICENSE | |||
* file that was distributed with this source code. | |||
* | |||
* @author Sacha Telgenhof <stelgenhof@gmail.com> | |||
* @author Quentin Neyrat <quentin.neyrat@gmail.com> |
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.
Can you leave my name here unchanged? You can add yours in the comment above the class definition like this:
/**
* Class InvalidYearException.
*
* @author Quentin Neyrat <[email protected]>
*/
class InvalidYearException extends InvalidArgumentException implements Exception
{
@qneyrat Can you please review your PR as there are some unanswered questions? If I don't get any feedback by the end of next week, I will close this PR. |
it's ready for me :) |
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.
Looking great now! Can you add also unit tests that assert these new exceptions are thrown correctly. Please also update the changelog that reflect this PR :)
Hello, I don't have time to improve this PR for so that it is ready to merge. Anyone like to improve code? Else, I can close that. |
@qneyrat No worries. I will pick up from here. Anyways, thanks for the contribution! |
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.
Changes are good. Unit tests need to be adjusted where applicable.
No description provided.