search

darrynten / clarifai-php

Clarifai API client for PHP
MIT License
25 stars 15 forks source link

Code review fixes #5

Closed mxnr closed 7 years ago

mxnr commented 7 years ago

Hello Darryn! I've reviewed the code and made some small changes related to code style. Couldn't not run unit tests from dev branch, i've catch missed array keys in mock objects, thats why i create DataHelper trait and isolate mock related data into it. Does it looks good for you? Thanks!

darrynten commented 7 years ago

Thank you so much for your great PR. It is most appreciated.

Regarding the comment about the spaces around the dots, please see my reasoning to this request.

I have made some comments and look forward to your feedback.

darrynten commented 7 years ago

I have pushed a commit to this PR that adds spaces back around concatenation dots (please see my reason)

I also updated the CONTRIBUTING file with this guideline, and have added and credited you in the README.md

Thanks again for the awesome work.

mxnr commented 7 years ago

Regarding the dots-spaces in concatenation (sorry, for some reason i cannot find the comment here), this is a standard from Symfony world. But i think it's up to you and ok to have spaces there. This is not an important as the PSR rule and different projects allows different usage of concatenation delimiters (I remember that Zend, Doctrine, Drupal use spaces).