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

Patch now_is_between #8

Merged
merged 4 commits into from
Dec 24, 2018
Merged

Patch now_is_between #8

merged 4 commits into from
Dec 24, 2018

Conversation

danobot
Copy link
Contributor

@danobot danobot commented Dec 17, 2018

Python throws error when app uses this function.

apps/LightingSM.py:242: in on_enter_active
    if self.is_night():
apps/LightingSM.py:201: in is_night
    return self.now_is_between(self.night_mode['start_time'], self.night_mode['end_time'])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <apps.LightingSM.LightingSM object at 0x7f7530171eb8>, start_time_str = '20:00:00', end_time_str = '20:00:00', name = None

    def now_is_between(self, start_time_str, end_time_str, name=None):
>       return self.AD.now_is_between(start_time_str, end_time_str, name)
E       AttributeError: 'LightingSM' object has no attribute 'AD'

/usr/local/lib/python3.6/site-packages/appdaemon/appapi.py:300: AttributeError

Hopefully I can mock it, though getting the function itself to work would be better. Any idea why Python may not be able to find the AD object?

given_that.passed_arg('night_mode').is_set_to(night)
    given_that.time_is(time(hour=19))

    ml.initialize()
    given_that.mock_functions_are_cleared()

    ml.sensor_state_change(SENSOR_ENTITY, None, 'off', 'on', None)
    ml.sensor_state_change(SENSOR_ENTITY, None, 'on', 'off', None)
    

    assert_that(CONTROL_ENTITY).was.turned_on(brightness=100)
    assert ml.previous_delay == 180
    given_that.time_is(time(hour=20))
    given_that.mock_functions_are_cleared()
    

    ml.sensor_state_change(SENSOR_ENTITY, None, 'off', 'on', None)

The only strange thing I am doing (is it strange?) is setting new values using given_that and then calling given_that.mock_functions_are_cleared() again (without calling initialize()

@HelloThisIsFlo
Copy link
Owner

Hi,

The way this framework is implemented is by patching some functions of Hass (like the one you just added). For the functions not patched, it'd use the real implementation. The rationale behind it was basically the same the idea you had: "It would be better getting the function itself to work". So only patching some, and using the real implementation for some . . .

. . . Unfortunately, that use-case never became real. I could never find a function that would work on its own out of the box without some serious reverse engineering of the way it's implemented. I went down that rabbit hole a couple of times, but eventually, each time gave up and tried to mock it as closely as possible to the real implementation. You can see this at work in the following cases:

  • State management: given_that.state_of and get_state
  • Time management: givent_that.time_is, time_travel.fast_forward and time_travel.assert_current_time

That AD object seems to be the core of Appdaemon, which is not initialized in the framework. Retrospectively thinking, probably getting that to work would be the key to make some functions work out of the box. But for some reason, I haven't investigated that road back then.

So, so far, the proper way to get these functions to work is to:

  1. Patch them
  2. Find a way to integrate them seamlessly with the framework

You got that first part right, from there a nice implementation would be to integrate it with the time_travel module, so that run_in, now_in_between and time_travel.assert_current_time would all be coherent

Copy link
Owner

@HelloThisIsFlo HelloThisIsFlo left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

The patch part is straightforward, I'd be merging this. However, I can't merge the changes to the README. At least not as is.
Please read my comments and let me know if they make sense.

Also if you have any feedback on how to improve my communication it'd be very welcome. I always wish to express my point of view in a way that's as clear and comprehensive as possible, but I'm often worried I sound too harsh, especially in written form.

In any case, I'm really grateful you're spending some time to make this project better. You're awesome 😃

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@HelloThisIsFlo HelloThisIsFlo merged commit ef7122d into HelloThisIsFlo:master Dec 24, 2018
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.

None yet

2 participants