Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update index.js #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PhilippLeitner
Copy link

Set anyoneSensor defalt to false. Fix for Issu #78

Set anyoneSensor defalt to false
@DanClarke-io
Copy link

DanClarke-io commented Jan 26, 2018

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...

@PhilippLeitner
Copy link
Author

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.

@DanClarke-io
Copy link

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants