-
Notifications
You must be signed in to change notification settings - Fork 54
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
Fix supperflous fields error bunq/sdk_php#118 #125
Fix supperflous fields error bunq/sdk_php#118 #125
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.
Remove the author headers .
use bunq\test\BunqSdkTestBase; | ||
|
||
/** | ||
* @author Kevin Hellemun <khellemun@bunq.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.
This header can be removed.
use bunq\test\BunqSdkTestBase; | ||
|
||
/** | ||
* @author Kevin Hellemun <khellemun@bunq.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.
header can be removed.
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.
src/Model/Core/BunqModel.php
Outdated
const FORMAT_STRING_EMPTY = ''; | ||
const SUFFIX_REQUEST_FIELD = '_field_for_request'; | ||
|
||
|
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.
Remove.
use bunq\Model\Generated\Object\Pointer; | ||
use bunq\test\BunqSdkTestBase; | ||
|
||
/** |
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.
This is so ugly. Why don't we leave it out when we don't adhere strictly to internal codestyle guidelines anyway?
* Test constants | ||
*/ | ||
const PAYMENT_CURRENCY = 'EUR'; | ||
const PAYMENT_DESCRIPTION = "php sdk Batch test"; |
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.
Single quotes rather than double qoutes.
*/ | ||
public function testSendBatchPayment() | ||
{ | ||
$response = PaymentBatch::create( |
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.
Fits on one line
{ | ||
$allPayment = []; | ||
|
||
while (count($allPayment) < 10) { |
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.
Magic value
@@ -45,4 +35,14 @@ public static function tearDownAfterClass() | |||
BunqContext::getApiContext()->resetSession(); | |||
BunqContext::getApiContext()->save(self::FILE_PATH_CONTEXT_CONFIG); | |||
} | |||
|
|||
/** | |||
* Delete's the current session. |
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.
Deletes
/** | ||
* Delete's the current session. | ||
* | ||
* This test has no assertion as of its testing to see if the code runs without errors. |
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.
as it is testing whether the code runs without errors.
Same typo as in other PR. Generated/copied error?
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.
Nah, back in the day when we were using the old PhpUnit you could leave tests without assertions hence the message in the test header. Since #96 it will say risky tests if no assertions where added. But I seem to forgot to remove these comments 🤦♂️
This PR closes/fixes the following issues:
must be merged after #124