search

TeleSign / php_telesign

The Telesign PHP SDK lets you easily integrate with our REST API.
MIT License
15 stars 33 forks source link

Code Improvement #9

Closed phplicengine closed 8 years ago

phplicengine commented 8 years ago

smart method was missing in your code, we added it. Also having 3 classes in the same file has autoloader problem, unless otherwise end-user split them, but why bother? just add all of them in the same class. also we suggest to have one options array parameter for everything, why having several parameters for language, phone_code etc. Hoewever there are many ways to Rome, but we think this is a bit improvement. We hope you find it useful. This is the PR we said in https://github.com/TeleSign/php_telesign/issues/8 we are not sure if it did worth our time to improve and submit it as it appears TeleSign should have no users at all because of bad management by some impolite staff or maybe we should expect even insult from them for this PR.

cmccolgan commented 8 years ago

Hello, thank you for the pull request. If you'd like us to consider this request here are some high level guidelines that you should follow:

  1. Please don't remove license terms in your pull request.
  2. For pull requests please edit files in the pull don't delete and then re-add them.
  3. Please don't remove line breaks, reformat (change tabs to spaces), or do other transformations on files that would otherwise cause diffing versions not to work as this makes viewing changes time consuming.

While we always like to have people contribute to our SDK's please do so in a manner that allows for easy review. A good set of tips to follow can be found here:

http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/

While there are many other guides out there certainly the "avoid re-formatting" is something to especially take note of.

Thanks.

phplicengine commented 8 years ago

Hello,

Actually we are always doing the way you said. We have no idea why this time it was one large line instead of indentation and spaces, probably a temporary server glitch. Anyway, if we got some free time to waste we will do another pr as we are no longer interested in TeleSign as staff were impolite with us, it seems staff are just a few impolite school kids or may be TeleSign is actually a fake company.

cmccolgan commented 8 years ago

So as not to cause you to waste any time I did a little parsing so I could look more at the changes suggested. As for the one options array parameter it certainly makes the code cleaner but as you say there are multiple ways to Rome. The thought there on our end was to make the call as explicit as possible and by enumerating the options doing that. Sort of a "belt and suspenders" approach but we'll probably leave it that way. As for the 3 classes in the same file I'll chat with our PHP guys about that.

Sorry to hear you had a bad support experience. If you'd like to email me about that directly please do I can be reached at cm at telesign dot com. I like to keep git hub comments and submissions about code.

Thanks.

phplicengine commented 8 years ago

Thanks for allowing us to email you. An email already sent. Unlike your dumb colleagues you seem to be a bit more polite than them!