Refactoring, mainly docblocks and CS. #62
Conversation
|
Thank you for contributing @rayrutjes! I'll let @maxiloc take over regarding the review. The tests are failing because the API keys are (indeed) not injected, don't worry. |
|
My pleasure @redox. Regarding the API keys I get the fact that you do not want to pollute Agolia's servers with test traffic. Maybe we could isolate the actual transport layer of the client so that we can mock it when doing the tests. AWS PHP's SDK has a nice approach illustrating what I am saying, here is an interesting snippet: https://github.com/aws/aws-sdk-php/blob/0f54c98d293afda9cff7575227da2a19851a08e5/tests/UsesServiceTrait.php This would also address #48 by the way, and lower the required time to run the test suite from a few minutes to a few seconds. I do realize though that this would require quite some work, and that it would probably imply bumping to a new major release. |
| $queryParameters['tagFilters'] = $tagFilters; | ||
| } | ||
| if ($userToken != null && strlen($userToken) > 0) { | ||
| $queryParameters['userToken'] = $userToken; | ||
| } | ||
| $urlEncodedQuery = Client::buildQuery($queryParameters); | ||
| $urlEncodedQuery = self::buildQuery($queryParameters); |
There was a problem hiding this comment.
I think static would make even more sense here
There was a problem hiding this comment.
You are right, this would eventually allow a child class to override the buildQuery method.
|
@rayrutjes Looks good, I did two comments tell me what you think. |
| @@ -0,0 +1,12 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Do we need this file in the repo ?
There was a problem hiding this comment.
If we leave this in the repo, we allow everyone to just run php-cs-fixer fix and ensure that everyone as the same code styles.
Just a suggestion though. Could be added as a good practise to the contributing guidelines of the repo.
|
Hi @maxiloc , could you take a look at what has be done so far and tell me how you feel about it? |
| @@ -41,7 +41,7 @@ public function testSettingsIndex() | |||
|
|
|||
| public function testSearchFacet() | |||
| { | |||
| $res = $this->index->addObjects(array( | |||
| $this->index->addObjects(array( | |||
| array("firstname" => "Robin"), | |||
There was a problem hiding this comment.
We can go for [] syntax here
|
Except the [] syntax of my two comments, looks really good, after this, we can merge if you're done with this PR. |
|
I made php-cs-fixer also correct the tests folder. The tests runs fine on my side. Do you have a way of running the tests on this pr before merging? |
|
@rayrutjes Yes I am going to test it right now. Can you rebase your commits in the meantime ? |
|
All green ! |
rayrutjes
force-pushed
the
code-styles-suggestion
branch
from
b560841
to
f7d5fb7
Compare
rayrutjes
force-pushed
the
code-styles-suggestion
branch
from
f7d5fb7
to
2d0fcb2
Compare
|
Sorry for the delay, I have rebased the pr. |
|
Ok cool ! LGTM |
|
Waiting for test to pass on #65 |
rayrutjes
changed the title
|
@rayrutjes Thanks for contributing. Much appreciated |
Started correcting some docblocks, and refactored some small pieces.
Please note that I have not a deep understanding of the behaviour of this client yet, so please tell me if some changes may break something. Hope the tests will catch problems though.
Do you guys see any interest in these adjustments if they do not introduce BC issues?
Please react on the changes so I can adjust the PR.
Fixes #32