Skip to content

fix: remove checks for state for sentry mode#66

Merged
alandtse merged 5 commits into
zabuldon:masterfrom
hobbe:fix/state-sentry-mode
Mar 22, 2020
Merged

fix: remove checks for state for sentry mode#66
alandtse merged 5 commits into
zabuldon:masterfrom
hobbe:fix/state-sentry-mode

Conversation

@hobbe
Copy link
Copy Markdown
Contributor

@hobbe hobbe commented Mar 17, 2020

Similar to PR #64, and as requested in PR #61, remove check for current state in case it is not accurate.

@alandtse
Copy link
Copy Markdown
Collaborator

Given the request in the main HA, I'll leave this open for any further fixes we can batch together.

@hobbe
Copy link
Copy Markdown
Contributor Author

hobbe commented Mar 19, 2020

Regarding the issue in HA PR home-assistant/core#32938, we only know that sentry mode is available when we query vehicle_state, that is when update() is called.
This means that update needs to be called before any setup in HA.

@alandtse
Copy link
Copy Markdown
Collaborator

alandtse commented Mar 20, 2020

I think you can just set the entity_registry_enabled_default parameter to False on init and then enable it once the car is queried and we know. Probably makes sense to add that parameter to vehicle.py as True for default. In sentry_mode.py you can then add the KeyError check on async_update.

EDIT: Actually, that won't work because the entity itself won't run async_update after it's disabled. I think perhaps you do a data.get('sentry_mode') which will set the value to None which is unavailable in HA.

            self.__sentry_mode = data.get('sentry_mode') is True if data.get('sentry_mode') else None

But that's just off the top of my head and it looks ugly.

Users will then have to choose to disable it if they don't want the unavailable item. That will also suppress the KeyError.

EDIT 2: KeyError catch probably will look cleaner to be honest. Should also set the default value to None.

@hobbe
Copy link
Copy Markdown
Contributor Author

hobbe commented Mar 21, 2020

@alandtse , I have added a flag to know if sentry mode is available. If not, nothing should happen and sentry mode is always False.

As long as there is no flag at the vehicle level, then the switch will exist but do nothing. Can a switch be set to disabled?

Copy link
Copy Markdown
Collaborator

@alandtse alandtse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as there is no flag at the vehicle level, then the switch will exist but do nothing. Can a switch be set to disabled?

Yes with the available property.

Comment thread teslajsonpy/sentry_mode.py Outdated
Comment thread teslajsonpy/sentry_mode.py
Comment thread teslajsonpy/sentry_mode.py
hobbe and others added 2 commits March 22, 2020 11:29
feat: add sentry mode availability flag
change key check
rename is_available to available
rename is_enabled to is_on
remove get_value
@alandtse alandtse merged commit d1f9199 into zabuldon:master Mar 22, 2020
@hobbe
Copy link
Copy Markdown
Contributor Author

hobbe commented Mar 22, 2020

Argh... I am too late. I removed the unecessary __sentry_mode_available, as it is already available in VehicleDevice as sentry_mode_available. I keep it for next PR.

@alandtse
Copy link
Copy Markdown
Collaborator

Ooops. I thought you were done since you addressed everything. If you know anything about using pytest, it'd be nice to have a suite of tests for sentry mode. That may be a worthwhile PR to add and then we can remove it in that one too.

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