Skip to content
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

Merged
merged 7 commits into from
Jun 1, 2019
Merged

Add new exceptions in Yasumi. #95

merged 7 commits into from
Jun 1, 2019

Conversation

qneyrat
Copy link
Contributor

@qneyrat qneyrat commented Feb 22, 2018

No description provided.

Copy link

@rmasclef rmasclef left a 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

@qneyrat
Copy link
Contributor Author

qneyrat commented Feb 22, 2018

@stelgenhof you prefer me in author or you ?

@stelgenhof stelgenhof self-requested a review February 22, 2018 15:19
@stelgenhof
Copy link
Member

@qneyrat You can put yourself as an author in the Class comments section :) and leave the file comments as is.

Copy link
Member

@stelgenhof stelgenhof left a 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?

@qneyrat
Copy link
Contributor Author

qneyrat commented Feb 22, 2018

To catch specific exception and not invalidArgument is so generic.
I want to be able to catch only UnknownIso31662Exception for example.

@stelgenhof stelgenhof changed the base branch from master to develop March 6, 2018 01:52
src/Yasumi/Yasumi.php Outdated Show resolved Hide resolved
@qneyrat
Copy link
Contributor Author

qneyrat commented Mar 23, 2018

@stelgenhof are you ready to merge ?

@stelgenhof
Copy link
Member

@qneyrat I believe there are some unanswered questions in the review of your PR.

@rmasclef
Copy link

@stelgenhof which ones ?

@stelgenhof
Copy link
Member

@rmasclef There is a small conversation regarding the use of the UnknownIso31662Exception (https:/azuyalabs/yasumi/pull/95/files/189127ef36697c2e230900d505b532f8efa8b6e6#diff-76dc48f17f4e7918c5844a2c6960d00cL189).

To me it looks like it is not concluded :)

@rmasclef
Copy link

@stelgenhof indeed, I agree with that one 👍

src/Yasumi/Yasumi.php Outdated Show resolved Hide resolved
Copy link
Member

@stelgenhof stelgenhof left a 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>
Copy link
Member

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
 {

src/Yasumi/Yasumi.php Outdated Show resolved Hide resolved
@stelgenhof
Copy link
Member

@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.

@qneyrat
Copy link
Contributor Author

qneyrat commented May 15, 2018

it's ready for me :)

Copy link
Member

@stelgenhof stelgenhof left a 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 :)

@qneyrat
Copy link
Contributor Author

qneyrat commented May 29, 2019

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.

@stelgenhof
Copy link
Member

@qneyrat No worries. I will pick up from here. Anyways, thanks for the contribution!

@stelgenhof stelgenhof self-assigned this May 29, 2019
Copy link
Member

@stelgenhof stelgenhof left a 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.

@stelgenhof stelgenhof merged commit 48bb8c5 into azuyalabs:develop Jun 1, 2019
@qneyrat qneyrat deleted the improve-exceptions branch June 3, 2019 13:07
@qneyrat qneyrat restored the improve-exceptions branch June 3, 2019 13:07
@qneyrat qneyrat deleted the improve-exceptions branch June 3, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants