homebridge-people icon indicating copy to clipboard operation
homebridge-people copied to clipboard

Update index.js

Open PhilippLeitner opened this issue 7 years ago • 4 comments

Set anyoneSensor defalt to false. Fix for Issu #78

PhilippLeitner avatar Jan 18 '18 11:01 PhilippLeitner

I'm not sure if this is correct, the default according to the readme is true for anyoneSensor should be true. Because of #78, do you think the default should be false for both then?

Additionally, this would break every implementation where the default hadn't been changed. I don't think this can be merged in...

DanClarke-io avatar Jan 26 '18 12:01 DanClarke-io

yes, i think it should be false for both of them. if it is false in the source, the config will set it true if it is necessary. if it es true, the config can not set it to false.

in my case, this change fixed the #78 .

the default in the config shuld be true and the default in de index.js shuld be false. this shuld not break anything, because of the config file, witch hase true as default.

PhilippLeitner avatar Feb 05 '18 13:02 PhilippLeitner

I understand why you think the default should be true false for both - however people who already have this implemented, who have not set anyoneSensor in their config (as it is optional), it would now change that to true, changing their current setup.

i.e. if my config is this:

"platforms": [
    {
        "platform": "People",
        "people" : [
            {
                "name" : "Pete",
                "target" : "PetesiPhone",
            },
        ]
    }
]

Then under the current config, the anyoneSensor would default to true, and the above config would create an Anyone Sensor. However if your PR was implemented, and people updated the plugins (which people do periodically) then the Anyone Sensor would stop working - therefore changing/breaking implementations.

Does that make sense, or am I confusing myself?

DanClarke-io avatar Feb 05 '18 14:02 DanClarke-io

Sorry, I now completely understand (after re-reading #88) - however your fix will break other implementations. I've submitted another PR (#92) which should fix your implementation issues.

To try this out, run npm install -g git+https://github.com/OverByThere/homebridge-people.git

Note: this will add in some firewalled options from an idea in #74, it shouldn't affect your install though.

DanClarke-io avatar Feb 05 '18 14:02 DanClarke-io