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

Fix all entities triggering all observations in bayesian sensor #28979

Merged
merged 8 commits into from Nov 26, 2019

Conversation

@sophof
Copy link
Contributor

sophof commented Nov 23, 2019

Description:

Fixed a bug caused in creating a dictionary of entities to observe. Using dict.fromkeys all entities where observed for each observation since the empty list was added by reference. Replaced by dict comprehension.
Also using this to learn how to commit my work preparing for more substantial changes to this component.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

tox basically takes forever, but it finished in the end. It's not very useful for small pull requests such as this one though, it took more than two hours!
Have also run all tests manually as mentioned in https://developers.home-assistant.io/docs/en/development_testing.html, this was much faster, is this enough for future pull-requests?

(removed all non-applicable checklists)

sophof added 7 commits Nov 22, 2019
…list by reference. Now each entity id has its own seperate list
…list by reference. Now each entity id has its own seperate list
…list by reference. Now each entity id has its own seperate list

for ind, obs in enumerate(self._observations):
obs["id"] = ind
if "entity_id" in obs:
self.entity_obs[obs["entity_id"]].append(obs)
self.entity_obs.get(obs.get("entity_id")).append(obs)

This comment has been minimized.

Copy link
@balloob

balloob Nov 26, 2019

Member

Let's keep this as []. get has slightly different behavior in that it returns None instead of raising KeyError. Since we know it exists, we should use [].

This comment has been minimized.

Copy link
@sophof

sophof Nov 26, 2019

Author Contributor

Ah, I figured using get was better because of that, but I guess that'll fail silently and you want the exception? I'll put it back as it was originally.

This comment has been minimized.

Copy link
@balloob

balloob Nov 26, 2019

Member

Yeah, failing silently is a pain, hard to find.

Copy link
Member

balloob left a comment

ok to merge when small comment addressed ! Great first contribution! Welcome to the Home Assistant community ⭐️ 🐬

Dev automation moved this from Needs review to Reviewer approved Nov 26, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 26, 2019

I recommend running script/lazytox.py for faster lint and test feedback.

@balloob balloob merged commit 078a7e4 into home-assistant:dev Nov 26, 2019
9 checks passed
9 checks passed
CI #20191126.82 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
Dev automation moved this from Reviewer approved to Done Nov 26, 2019
@sophof sophof deleted the sophof:fix_entity_obs branch Nov 26, 2019
@lock lock bot locked and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.