search

norkunas / onesignal-php-api

OneSignal API for PHP
MIT License
233 stars 82 forks source link

Add unit tests + refacto #74

Closed adelplace closed 6 years ago

adelplace commented 6 years ago

The goal here is to provide a pretty full coverage with unit tests in order to create a first stable version of the library.

Feel free to discuss about this work ;)

norkunas commented 6 years ago

For v1 this will be good, but for v2 I think it's worth to drop the options resolvers and just implement payload objects (DTO's), increase php version to 7.1 and use strict types

adelplace commented 6 years ago

Hi,

Thanks for the review ;) Concerning the comments on attributes I wouldn't say they're unecessary as they are recognized by IDEs, they help for autocompletion. Anyway I removed the one you pointed, tell me if it's not enough. About the idea to implement playload objects, I totally agree. It 's a way more user friendly.

norkunas commented 6 years ago

IDE's are smart enough to recognize the provided objects because the constructor has the same typehints, this only helps where constructor parameters are not objects, so I was trying to say that other docs could also be removed :)

adelplace commented 6 years ago

I have to admit that you're right, it should be better now.

adelplace commented 6 years ago

Hello,

Please tell me if there is a way to get this PR accepted and mostly if you plan to create a stable version of this project after that.

Thanks

norkunas commented 6 years ago

Hi, will review again this week and if ok will release v1

norkunas commented 6 years ago

Thank you @adelplace

adelplace commented 6 years ago

You're welcome !