Feature/exception factory #50

OGKevin · 2017-09-21T08:10:16Z

Exception factory review
Explain exceptions in a .md file
Final review

dnl-blkv

A few comments

dnl-blkv · 2017-09-21T09:05:18Z

src/Exception/ApiException.php

@@ -3,6 +3,35 @@

 /**
 */
-class ApiException extends BunqException
+class ApiException extends \Exception


Let us please do use Exception; and then Exception with no preceding \ :)

dnl-blkv · 2017-09-21T09:07:08Z

src/Exception/ExceptionFactory.php

+     * @param string $message
+     * @param array $args
+     *
+     * @return BadRequestException|ForbiddenException|MethodNotAllowedException|NotFoundException|PleaseContactBunqException|ToManyRequestsException|UnauthorizedException|UnknownApiErrorException


Just ApiException would be good :P

dnl-blkv · 2017-09-21T09:07:56Z

src/Exception/ToManyRequestsException.php

+
+/**
+ */
+class ToManyRequestsException extends ApiException


One o refuged it seems.

OMG this is in all the SDK's 😁 🙈

dnl-blkv · 2017-09-21T09:12:01Z

src/Exception/ApiException.php

+     *
+     * @param int $responseCode
+     */
+    public function __construct($message, $args = [])


Please add strict types for arguments (PHP 7.0 style) and doc block params. Also, why are we using $args? Is it necessary here? If so, please add a comment explaining this to the constructor header.

dnl-blkv · 2017-09-21T09:12:14Z

src/Exception/ApiException.php

+    /**
+     * @return int
+     */
+    public function getResponseCode()


Please add PHP 7.0 strict response type (: int)

dnl-blkv · 2017-09-21T09:12:46Z

src/Exception/ExceptionFactory.php

+     *
+     * @return BadRequestException|ForbiddenException|MethodNotAllowedException|NotFoundException|PleaseContactBunqException|ToManyRequestsException|UnauthorizedException|UnknownApiErrorException
+     */
+    public static function createExceptionForResponse($message, $args = [])


Same question: do we need args here?

Also please add strict param types and the response type :)

dnl-blkv · 2017-09-21T09:16:06Z

src/Http/Handler/ResponseHandlerError.php

@@ -60,15 +61,15 @@ public function execute(ResponseInterface $response)
                    $errorDescriptions[] = $error[self::FIELD_ERROR_DESCRIPTION];
                }

-                throw new ApiException(
+                throw ExceptionFactory::createExceptionForResponse(
                    self::ERROR_FROM_JSON,


I know it is not yours, but please make the interface of createExceptionForResponse consistent with that in all the other SDKs if possible :)

dnl-blkv · 2017-09-21T09:17:44Z

src/Http/Handler/ResponseHandlerError.php

                    self::ERROR_FROM_JSON,
                    [
                        $response->getStatusCode(),
                        implode(self::SEPERATOR_ERROR, $errorDescriptions),
                    ]
                );
            } else {
-                throw new ApiException(
+                throw ExceptionFactory::createExceptionForResponse(
                    self::ERROR_UNKNOWN,


Also, please make error handling here consistent with what we have in e.g. Python :)

dnl-blkv

A few more comments :D

dnl-blkv · 2017-09-21T12:53:47Z

src/Exception/ApiException.php

+    private $responseCode;
+
+    /**
+     * ApiException constructor.


This comment can go :)

dnl-blkv · 2017-09-21T12:54:02Z

src/Exception/ApiException.php

+     *
+     * @param int $responseCode
+     */
+    public function __construct($message, $responseCode)


Please add strict types and docs for both parameters!

Crap, I’m really getting old indeed xD

dnl-blkv · 2017-09-21T12:54:35Z

src/Exception/BunqException.php

     */
-    public function __construct($message, $args = [])
+    public function __construct($message, $responseCode = [])


Please add strict types for both parameters; Also $responseCode shouldn't be equal to an array I believe :D

dnl-blkv · 2017-09-21T12:59:39Z

src/Exception/ExceptionFactory.php

+    const INDEX_FIRST = 0;
+
+    /**
+     * @param array $messages


Please put the strict type here :)

dnl-blkv · 2017-09-21T12:59:48Z

src/Exception/ExceptionFactory.php

+     *
+     * @return ApiException
+     */
+    public static function createExceptionForResponse($messages, $responseCode)


tyyypes (both params and return ones)

dnl-blkv · 2017-09-21T13:00:08Z

src/Http/ApiClient.php

@@ -188,7 +188,7 @@ private function initializeHttpClient()
                    self::OPTION_HANDLER => $middleware,
                    self::OPTION_VERIFY => true,
                    self::OPTION_CURL => [
-                        CURLOPT_PINNEDPUBLICKEY => $this->determinePinnedServerPublicKey(),
+                        //CURLOPT_PINNEDPUBLICKEY => $this->determinePinnedServerPublicKey(),


Wut? Why is it commented out? :D

Shit yea haha, m y curl doesn’t work neither and didn’t fix it 😁. Therefore I must comment this line out for the sdk to work. Obviously forgot to revert it.

OGKevin · 2017-09-26T18:02:26Z

@dnl-blkv please 👀

dnl-blkv

Code is looking good, but please add the same changes as in C# to the text. Would be the best if you do so after C#, once we have the C# part merged and consolidated :)

dnl-blkv

One comment, and also please do add context file deletion after session deletion :)

dnl-blkv · 2017-10-06T10:33:08Z

src/Exception/EXCEPTION.md

+     * @param int $responseCode
+     */
+    public function __construct(string $message, int $responseCode)
+    {}


Please make it clear that some code is hidden here :)

OGKevin · 2017-10-07T10:14:46Z

@dnl-blkv all yours 🎖

dnl-blkv

A few styling comments... :D

dnl-blkv · 2017-10-09T15:38:15Z

src/Exception/EXCEPTION.md

+## Exceptions
+When you make a request via the SDK, there is a chance of request failing
+due to various reasons. When such a failure happens, an exception
+corresponding to the error occurred is raised.


Exceptions are thrown in PHP, not raised! :)

dnl-blkv · 2017-10-09T15:38:55Z

src/Exception/EXCEPTION.md

+corresponding to the error occurred is raised.
+
+
+----


Do we need these ---- ?

This makes a it more beautiful :P Its like a paragraph separator. View it as an actual markdown and you shall see 😁

dnl-blkv · 2017-10-09T15:39:38Z

src/Exception/EXCEPTION.md

+}
+```
+The `Exception` class which is being extended has an `getMessage()` method which is `final` and therefore cannot be
+overwritten.


"overridden" please :P

dnl-blkv · 2017-10-09T15:40:28Z

src/Exception/EXCEPTION.md

+    //Make a call that might fail
+    $apiContext = ApiContext::create(BunqEnumApiEnvironmentType::SANDBOX(), API_KEY, DEVICE_DESCRIPTION);
+} catch (BadRequestException $error){
+    //Do something if exception is thrown.


It is a minor thing, but here and in all the inline comments (// Comment) please make sure there is space between // and the first letter!

dnl-blkv · 2017-10-09T15:41:17Z

tests/Model/Generated/Endpoint/SessionTest.php

@@ -31,6 +36,6 @@ public function testDeleteSession()
     */
    public static function tearDownAfterClass()
    {
-        static::ensureApiContextValid();
+        unlink(static::FILENAME_CONTEXT_CONFIG);


Oops! I must have missed it at some earlier point. Let us please use self:: for constants :)

dnl-blkv

couple more :P

dnl-blkv · 2017-10-09T18:12:51Z

src/Exception/EXCEPTION.md

+     */
+    public function __construct(string $message, int $responseCode)
+    {
+        //Some hidden code


Space after // please! :)

dnl-blkv · 2017-10-09T18:12:56Z

src/Exception/EXCEPTION.md

+const DEVICE_DESCRIPTION = 'This will cause BadRequestException to be thrown.';
+
+try{
+    //Make a call that might fail


Space after // please! :)

dnl-blkv · 2017-10-10T17:12:50Z

@andrederoos

DennisSnijder · 2017-10-10T17:59:36Z

@dnl-blkv @andrederoos new release? 😄

dnl-blkv · 2017-10-10T18:00:56Z

@DennisSnijder yep, planned for tomorrow :)

DennisSnijder · 2017-10-10T18:37:52Z

@dnl-blkv Awesome! thanks 👍

andrederoos · 2017-10-10T19:56:21Z

src/Http/Handler/ResponseHandlerError.php


-                $errorDescriptions = [];
+        if ($responseBodyInJson != false){


@OGKevin !==. lets not fall in the php-weak-typing-trap

Ahh 🙈 good 👁 !

OGKevin added 4 commits September 21, 2017 10:00

Base excaption handler done.

4fae1d7

Modified test.

5d7e1fb

Some cleaning 👏.

70a3293

More cleaning 👏.

7dcabdf

OGKevin added the enhancement label Sep 21, 2017

OGKevin assigned dnl-blkv and OGKevin Sep 21, 2017

OGKevin requested a review from dnl-blkv September 21, 2017 08:10

dnl-blkv suggested changes Sep 21, 2017

View reviewed changes

OGKevin added 5 commits September 21, 2017 13:23

Rewritten ResponseErrorHandler.

7599e8f

Some cleaning 👏.

47f7170

More cleaning 👏.

926fd64

Im getting old 😁.

84b7ba4

😭

554dd4c

dnl-blkv suggested changes Sep 21, 2017

View reviewed changes

OGKevin added 3 commits September 25, 2017 09:21

Some cleaning 🔥

b4a4bb6

Some markdown 📝.

c47a4cc

Added readme reference.

c4acc09

dnl-blkv suggested changes Sep 28, 2017

View reviewed changes

OGKevin added 2 commits October 5, 2017 17:46

Merge branch 'develop' into feature/exception-factory

b1f1528

Copywrtiting 📝.

96996a3

dnl-blkv suggested changes Oct 6, 2017

View reviewed changes

OGKevin added 3 commits October 6, 2017 12:34

Merge branch 'develop' into feature/exception-factory

5fda0df

Magic hidden code.

c58a94a

All tests passing ⭐.

33fe8a5

dnl-blkv suggested changes Oct 9, 2017

View reviewed changes

OGKevin added 2 commits October 9, 2017 20:09

Some cleaning 🔥

ed61b14

Added a little doc.

7b91a04

dnl-blkv suggested changes Oct 10, 2017

View reviewed changes

Changes based on review.

4f25c4a

dnl-blkv approved these changes Oct 10, 2017

View reviewed changes

dnl-blkv merged commit d724129 into develop Oct 10, 2017

dnl-blkv deleted the feature/exception-factory branch October 10, 2017 17:12

andrederoos reviewed Oct 10, 2017

View reviewed changes

OGKevin mentioned this pull request Oct 11, 2017

!== instead of != #54

Merged

Feature/exception factory #50

Feature/exception factory #50

Conversation

OGKevin commented Sep 21, 2017 • edited by dnl-blkv Loading

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Sep 26, 2017

dnl-blkv left a comment

Choose a reason for hiding this comment

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin commented Oct 7, 2017

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnl-blkv commented Oct 10, 2017

DennisSnijder commented Oct 10, 2017

dnl-blkv commented Oct 10, 2017

DennisSnijder commented Oct 10, 2017

Choose a reason for hiding this comment

OGKevin Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

OGKevin commented Sep 21, 2017 •

edited by dnl-blkv

Loading

OGKevin Oct 11, 2017 •

edited

Loading