search

sqmk / Phue

Philips Hue client for PHP
Other
203 stars 45 forks source link

Pull Request as discussed #105

Closed jonofe closed 8 years ago

jonofe commented 8 years ago

Changed the line delimiter to \n for all files and sent a pull request.

sqmk commented 8 years ago

PHP tests look great. Coding standards will need to be addressed.

Just like running tests, you can run ./vendor/bin/phpcs --standard=tests/phpcs.xml library/ from phue root directory to test coding standards. If that and phpunit tests passed, travis should pass as well.

The README still also needs to be updated to the original (plus some of your changes). You can change the broken gitdoc api URL and fix code examples with PHP 5.3 syntax, but everything else should remain if possible.

jonofe commented 8 years ago

I installed phpcs via pear. I neither have phpcs in ./vendor/bin nor phpcs.xml in tests. What did I wrong?

------ Originalnachricht ------ Von: "Michael Squires" notifications@github.com An: "sqmk/Phue" Phue@noreply.github.com Cc: "jonofe" andre.feld@anrath.net Gesendet: 23.03.2016 23:48:44 Betreff: Re: [Phue] Pull Request as discussed (#105)

PHP tests look great. Coding standards will need to be addressed.

Just like running tests, you can run ./vendor/bin/phpcs --standard=tests/phpcs.xml library/ from phue root directory to test coding standards. If that and phpunit tests passed, travis should pass as well.

The README still also needs to be updated to what it was. You can change the broken gitdoc api URL and fix code examples with PHP 5.3 syntax, but everything else should remain if possible.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

sqmk commented 8 years ago

If you are on an outdated version of composer, can you try composer install --dev from /path/to/Phue? code_sniffer is definitely included as a dependency, and running locally for me.

After that, from /path/to/Phue, running ./vendor/bin/phpcs --standard=tests/phpcs.xml library/ will either return nothing (success!), or a list of errors.

sqmk commented 8 years ago

Excellent. I'll be tagging this as v1.4.0, and creating a release branch release/1.x for all PHP 5.3+ changes, bug fixes going forward.

sqmk commented 8 years ago

Oops, the only thing left is fixes to README. This should be corrected to mostly what sqmk/Phue current README contains, with your code sample changes and other minor fixes. After that, I can merge this in!

jonofe commented 8 years ago

just updated the README.md and all checks are now running without errors. so, good to go ... :)

Do I have add a new pull request?

------ Originalnachricht ------ Von: "Michael Squires" notifications@github.com An: "sqmk/Phue" Phue@noreply.github.com Cc: "jonofe" andre.feld@anrath.net Gesendet: 24.03.2016 00:16:45 Betreff: Re: [Phue] Pull Request as discussed (#105)

Excellent. I'll be tagging this as v1.4.0, and creating a release branch release/1.x for all PHP 5.3+ changes, bug fixes going forward.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

sqmk commented 8 years ago

No need for a new PR. Your PR will pick up any new changes you commit automatically!

Things look good here. Merging now.

jonofe commented 8 years ago

perfect. thanks for your patience with github newbie... and sorry again for the "Interesting" start ;-)

------ Originalnachricht ------ Von: "Michael Squires" notifications@github.com An: "sqmk/Phue" Phue@noreply.github.com Cc: "jonofe" andre.feld@anrath.net Gesendet: 24.03.2016 00:22:40 Betreff: Re: [Phue] Pull Request as discussed (#105)

No need for a new PR. Your PR will pick up any new changes you commit automatically!

Things look good here. Merging now.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

sqmk commented 8 years ago

No worries. Good going on the changes and PR, and handling the tests and coding standards fixes!

jonofe commented 8 years ago

Just dropped packagist and github fork ... Wish u a nice remaining day ... need sleep now, it's 01:00 am in Germany :)

------ Originalnachricht ------ Von: "Michael Squires" notifications@github.com An: "sqmk/Phue" Phue@noreply.github.com Cc: "jonofe" andre.feld@anrath.net Gesendet: 24.03.2016 00:52:27 Betreff: Re: [Phue] Pull Request as discussed (#105)

No worries. Good going on the changes and PR, and handling the tests and coding standards fixes!

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

jonofe commented 8 years ago

Hi Michael,

after using the Phue class during the last two days, I found out, that there is a difference in behaviour among php 5.3 and php 5.6. When I call the function getColorTemp() for HUE devices other than Bulbs, there is not ColorTemp 'ct' attribute in the light data structure. On PHP 5.3 that results in an error "Light.php line: 369 | Undefined property: stdClass::$ct" while in php 5.6 (Phue 1.3.0) there's no output at all. Do you know why this could happen?

Additionally there might be a minor inconsistency with the value range of saturation and brightness, which according to the philips documentation is defined from 0...254. I think currently in Phue it is handled from 0..255. That could somehow lead into inconsistencies when writing and reading back these values. I don't know, whether this is already handeled.

BR, André

sqmk commented 8 years ago

Can you create a new ticket for this, and provide PHP configuration/ini settings for both PHP 5.3 and PHP 5.6? Also, please test Phue 1.4.0 in both PHP 5.3 and PHP 5.6, as that is the latest version.

Thanks.