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

Add observed entities to bayesian sensor #27721

Merged
merged 9 commits into from
Feb 25, 2020
Merged

Conversation

paolog89
Copy link
Contributor

@paolog89 paolog89 commented Oct 15, 2019

Description:

The Bayesian binary sensor currently shows only a list of [object Object] under the field observations:
image

This change will include the list of entities that have been observed:
image

Related issue (if applicable): N/A

Pull request with documentation for home-assistant.io (if applicable): N/A

Example entry for configuration.yaml (if applicable): N/A

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

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @paolog89,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@project-bot project-bot bot added this to Needs review in Dev Oct 15, 2019
@MartinHjelmare MartinHjelmare moved this from Needs review to Incoming in Dev Oct 17, 2019
@paolog89 paolog89 marked this pull request as ready for review October 18, 2019 15:06
@MartinHjelmare MartinHjelmare moved this from Incoming to Needs review in Dev Oct 18, 2019
@sophof
Copy link
Contributor

sophof commented Nov 23, 2019

@paolog89 This change would be really useful to me, thanks! Checking your code though I think it will duplicate entity ids that are relevant for multiple observations. You might want to create a unique set.

@paolog89
Copy link
Contributor Author

@paolog89 This change would be really useful to me, thanks! Checking your code though I think it will duplicate entity ids that are relevant for multiple observations. You might want to create a unique set.

@sophof Thanks for your feedback. Isn't that also a feature? This way you can see if you have multiple observations.

@sophof
Copy link
Contributor

sophof commented Nov 25, 2019

@paolog89 This change would be really useful to me, thanks! Checking your code though I think it will duplicate entity ids that are relevant for multiple observations. You might want to create a unique set.

@sophof Thanks for your feedback. Isn't that also a feature? This way you can see if you have multiple observations.

I guess so, but won't it be impossible to differentiate them anyway if an entity trigger multiple observations? I worry that especially with libera l use of templates (I'm working on an update that enhances templates) it'll get very spammy...

Copy link
Contributor

@dgomes dgomes left a comment

Choose a reason for hiding this comment

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

Tests need to pass but looks good

Dev automation moved this from Needs review to Reviewer approved Feb 2, 2020
@paolog89
Copy link
Contributor Author

paolog89 commented Feb 3, 2020

Tests need to pass but looks good

Thanks! Do I need to do something?

@springstan
Copy link
Member

@paolog89 you will need to format your code with black :)

@balloob
Copy link
Member

balloob commented Feb 9, 2020

We don't make changes to the backend based on a display issue in the frontend.

You're also removing an existing attribute and replace the content with something else, that does not match the name.

@balloob balloob closed this Feb 9, 2020
Dev automation moved this from Reviewer approved to Cancelled Feb 9, 2020
@paolog89
Copy link
Contributor Author

paolog89 commented Feb 9, 2020

We don't make changes to the backend based on a display issue in the frontend.

You're also removing an existing attribute and replace the content with something else, that does not match the name.

Hello Paulus, would it be ok to add an attribute with the observed entities? how would you solve the fronted issue?

@balloob
Copy link
Member

balloob commented Feb 9, 2020

Yes a new attribute is fine.

Open an issue on the frontend repo

@paolog89
Copy link
Contributor Author

paolog89 commented Feb 9, 2020

There is an issue on the frontend repo [#3689](https://github.com/home-assistant/home-assistant-polymer/issues/3689). Can you re-open it or should I open a new one?

Can you re-open this pull request or should I open a new one?

@balloob balloob reopened this Feb 10, 2020
Dev automation moved this from Cancelled to Incoming Feb 10, 2020
@balloob
Copy link
Member

balloob commented Feb 10, 2020

Done and done.

@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #27721 into dev will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #27721      +/-   ##
==========================================
+ Coverage   94.41%   94.64%   +0.22%     
==========================================
  Files         626      756     +130     
  Lines       47140    54840    +7700     
==========================================
+ Hits        44506    51901    +7395     
- Misses       2634     2939     +305     
Impacted Files Coverage Δ
homeassistant/components/recorder/migration.py 62.40% <0.00%> (-30.73%) ⬇️
homeassistant/components/google_assistant/http.py 79.16% <0.00%> (-9.97%) ⬇️
homeassistant/bootstrap.py 72.61% <0.00%> (-7.51%) ⬇️
homeassistant/components/ring/sensor.py 86.86% <0.00%> (-8.84%) ⬇️
homeassistant/components/ring/light.py 89.79% <0.00%> (-8.25%) ⬇️
homeassistant/components/updater/binary_sensor.py 92.10% <0.00%> (-7.90%) ⬇️
...istant/components/google_assistant/report_state.py 85.00% <0.00%> (-4.19%) ⬇️
homeassistant/__init__.py 88.46% <0.00%> (-7.43%) ⬇️
homeassistant/components/light/device_action.py 93.93% <0.00%> (-6.07%) ⬇️
homeassistant/components/mfi/switch.py 89.28% <0.00%> (-5.36%) ⬇️
... and 558 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c700085...7447797. Read the comment docs.

@paolog89 paolog89 requested a review from dgomes February 10, 2020 18:11
@MartinHjelmare MartinHjelmare moved this from Incoming to Review in progress in Dev Feb 13, 2020
@balloob
Copy link
Member

balloob commented Feb 21, 2020

You need to sort the output in your tests. Sets are not sorted.

@balloob balloob merged commit 0e6a48c into home-assistant:dev Feb 25, 2020
Dev automation moved this from Review in progress to Done Feb 25, 2020
@lock lock bot locked and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants