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 Pagination and missing fields/endpoints [#15] #43

Merged
merged 14 commits into from
Sep 6, 2017

Conversation

dnl-blkv
Copy link
Contributor

@dnl-blkv dnl-blkv commented Sep 1, 2017

Fixes #15

@dnl-blkv dnl-blkv self-assigned this Sep 1, 2017
@dnl-blkv dnl-blkv changed the title Add Pagination and missing fields/endpoints Add Pagination and missing fields/endpoints [#15] Sep 1, 2017
@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 1, 2017

@OGKevin Please check if it is good with PHP 5.6 and assign to @andrederoos once reviewed (don't merge yet!)

Yeeah, that's a lot :D

@OGKevin
Copy link
Contributor

OGKevin commented Sep 3, 2017

@dnl-blkv Did you mean php 5.6 ? If so, tests are failing due to #38.

@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 3, 2017

@OGKevin oh yep, that was 5.6!

Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments and questions 🤔

*/
protected $pinCodeAssignment;

/**
* ID of the MA to be used as fallback for this card if insufficient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not generated right ? 😁 Please spell out monetary account

Copy link
Contributor Author

@dnl-blkv dnl-blkv Sep 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OGKevin What makes you think it is not generated? src/Model/Generated/Card.php :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaahhhhh .gitattributes, will make PR tonight.

protected $count;

/**
* @param string[][] $paginationJson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a nested array correct ?

{
$this->pinCodeAssignment = $pinCodeAssignment;
}

/**
* ID of the MA to be used as fallback for this card if insufficient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Model/Generated/Card.php

protected $pinCodeAssignment;

/**
* ID of the MA to be used as fallback for this card if insufficient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Model/Generated/CardDebit.php

}

/**
* ID of the MA to be used as fallback for this card if insufficient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monterey account

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/Model/Generated/CardDebit.php

@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 4, 2017

@OGKevin Please take a look and assign to @andrederoos if it is OK :)

@OGKevin
Copy link
Contributor

OGKevin commented Sep 4, 2017

Ok LGTM, @andrederoos please review, but do not merge yet. Need to run tests after your review 🥇

@OGKevin
Copy link
Contributor

OGKevin commented Sep 4, 2017

@dnl-blkv all tests passing on 7.1, please include tests for pagination as we discussed.

@OGKevin OGKevin modified the milestones: patch 0.10.1, v0.11.0 Sep 5, 2017
@dnl-blkv
Copy link
Contributor Author

dnl-blkv commented Sep 6, 2017

@OGKevin this is now also ready for review; tests passing! Please take a look and then assign to @andrederoos

Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tests passing.

$headers = [];

foreach ($response->getHeaders() as $headerKey => $headerValues) {
$headers[$headerKey] = join(self::GLUE_HEADER_VALUE, $headerValues);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was not familiar with this alias, please use implode(). why php introduces aliases is a big mystery ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

/**
* @return Pagination|null
*/
public function getPagination()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPaginationOrNull? I think really helps making code breaks less... your call..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the problem with the absence of proper notion of nullability in PHP. With PHP 7.1 we will have it, and IDE will be able to say "hey, it can be null!". I'd just leave it as is because then with PHP 7.1 this won't need to be changed back.

In Java, for instance, we do not need to say orNull because IDEs do it for us :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

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.

None yet

3 participants