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

Updates for Appdaemon 4 and Python 3.8 #37

Conversation

chrisingrassia
Copy link

All this really entailed was removing the last "global args" argument from the Hass constructor

All this really entailed was removing the last "global args" argument from the Hass constructor
Copy link
Collaborator

@lifeisafractal lifeisafractal 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 submitting these fixes. Can you have a look at the two review comments I left above and let me know what you think. Once we get those ironed out we can get this merge. Also, your issues with Python3.8 prompted me to create #38 which proposes we state explicitly which Python versions we support and to get them under coverage so we catch this stuff with automated test in the figure. Please add a comment there if you have any thoughts and thanks again for taking the time to open a PR!

@@ -58,6 +58,8 @@ def entity_exists_mock(entity_id):
def _init_mocked_passed_args(self):
def make_magic_mock_behave_like_a_dict(magic_mock, dict_to_simulate):
def should_mock(dict_method):
if dict_method == "__reversed__":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch! According to this, it looks like dict now implements __reversed__ in Python3.8 which makes sense to me why this would break. This fix does address the issues, but how would you feel about a more general solution that would handle any case where an attribute of dict doesn't show up in the MagicMock. For example:

def should_mock(dict_method):
	dict_method_in_magic_mock = getattr(magic_mock, dict_method, None)
	if dict_method_in_magic_mock is None:
		return False
	return isinstance(dict_method_in_magic_mock, MagicMock)

@@ -111,7 +111,7 @@ def log_log(msg, level='INFO'):

def _mock_hass_init(hass_functions):
"""Mock the Hass object init and set up class attributes that are used by automations"""
def hass_init_mock(self, _ad, name, _logger, _error, _args, _config, _app_config, _global_vars):
def hass_init_mock(self, _ad, name, _logger, _error, _args, _config, _app_config):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this has been fixed on master since you branched off since the refactor that replaced init_framework.py with hass_mocks.py. Can you merge/rebase master and see if that's the case.

Copy link
Owner

Choose a reason for hiding this comment

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

➕1
This should be fixed already 👍

@HelloThisIsFlo
Copy link
Owner

@chrisingrassia Thank you very much for the fix.
The work as been merged to the repo via 2 different means:

  • A commit fixing support for appdaemon 4: 87fd783
    It was done before or at the moment you submitted your PR.
  • A PR fixing python 3.8: Fix bug python38 #40
    I went on a separate PR for this one because I wasnted to refactor the code instead of applying a hotfix.

So while I am going to close this PR and none of your work is going to be merged. I want to thank your for contributing, and I hope you understand why I decided to go with a custom solution. Your fix did really help identify the issue 😊

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

3 participants