Zn4rK / php-withings

PSR-FIG Compatible Withings Body metrics Services API written in PHP.
17 stars 14 forks source link

Support more API endpoints and reorganize measures #27

Closed u1735067 closed 6 years ago

u1735067 commented 6 years ago
Zn4rK commented 6 years ago

Nice!

I'll take a look ASAP, but it seems like you've done some extensive work here - awesome!

u1735067 commented 6 years ago

The only strong opinion I have is that we shouldn't use magic overloading if we don't need to, and I like the getters. The strongest reason for it, is that together with PHPDoc it becomes much more easy to work with and it increases the readability when you don't have to figure out what the magic method actually does.

I understand that. However, my "IDE" is Codiad, so I don't have autocompletion/whateverItIs to create the getters. I'm not going to create them by hand. The get() and call() I put are mostly for retrocompatibility, IMO, properties should be public (is there a reason to prevent "users" (developers) to access them ?), documented, and used directly.

We are also missing PHPDoc one a few places, and that needs to be addressed.

Yes, it's needed.

This library follows PSR-FIG PSR-2, and if you can make changes that reflect that I will gladly merge this!

Is there something else than what you mentionned that should be changed for compliance ?

Btw, what is the "Jebaited" thing ?

Zn4rK commented 6 years ago

I have no idea about the jebaited thing. A friend tried to be funny - I think. Sorry about that.

Zn4rK commented 6 years ago

Normally, I wouldn't directly disagree with you. I would probably set them at protected and have the developer extend and replace the class if needed anyway though. With the dependency injection in Laravel it's possible.

But since our data is coming from an external API we need to be sure that it hasn't changed by some third party library or that another developer in our project has taken a shortcut or made a logic error, or something else. With non-public properties and getters we can be sure that nothing has happened to our data.

Other than that, it's just a preference.

Zn4rK commented 6 years ago

How about this? I'll merge this into development, and I'll try to go over the code and add the getters...

Zn4rK commented 6 years ago

I like the debug constant idea!

u1735067 commented 6 years ago

Fixed the variable errors & the noted braces. Array notation, visibility and getters are left to you (they shouldn't prevent this version from working). The project I worked on planned to use this lib, but the project is no longer alive :/ I hope the changes will be useful to someone else though !

Zn4rK commented 6 years ago

Thank you for this! And I'm sorry to hear about your project... Merging it into development to give me time to adjust the code to my likings :)