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

Fixed bug in #169 #181

Closed
wants to merge 6 commits into from
Closed

Fixed bug in #169 #181

wants to merge 6 commits into from

Conversation

GeoDerp
Copy link
Contributor

@GeoDerp GeoDerp commented Feb 5, 2024

Related to, and possible fix for issue: davidusb-geek/emhass-add-on#74

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: Patch coverage is 98.61111% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 87.75%. Comparing base (c9686cd) to head (910c26d).
Report is 158 commits behind head on master.

❗ Current head 910c26d differs from pull request most recent head 682a1d7. Consider uploading reports for the commit 682a1d7 to get more accurate results

Files Patch % Lines
src/emhass/utils.py 98.61% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #181       +/-   ##
===========================================
+ Coverage   73.21%   87.75%   +14.53%     
===========================================
  Files           7        6        -1     
  Lines        2035     1707      -328     
===========================================
+ Hits         1490     1498        +8     
+ Misses        545      209      -336     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GeoDerp

This comment was marked as outdated.

@davidusb-geek
Copy link
Owner

This PR reinstates the associations list and dict and solves the issue that arised when we released this method. Is that right?

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Feb 7, 2024

In theory yes 👍

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

This PR will need to be tested to see if its still functional and dosn't generate any bugs.

@davidusb-geek
Copy link
Owner

This PR will need to be tested to see if its still functional and dosn't generate any bugs.

Ok, we will probably integrate this in a future 0.9.0 release along with the new MLRegression class.
But yes will need to be tested.

Also requirements_addon.txt file is no longer needed right?

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

This PR will need to be tested to see if its still functional and dosn't generate any bugs.

Ok, we will probably integrate this in a future 0.9.0 release along with the new MLRegression class.
But yes will need to be tested.

Also requirements_addon.txt file is no longer needed right?

Oh sorry. Yeah I don't believe so.
Does the web server requirements only being used for the devcontainer? If so, could that be removed also. (replaced with manual run pip installs?)

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

It may be good to keep the web server requirements if you want to specify the version of flask, waitress and ploty. I'll change the Docker to use the file if that's the case? @davidusb-geek

@davidusb-geek
Copy link
Owner

It may be good to keep the web server requirements if you want to specify the version of flask, waitress and ploty. I'll change the Docker to use the file if that's the case? @davidusb-geek

Yes we will keep the web server requirements and erase the other requirements_addon file

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Mar 2, 2024

It may be good to keep the web server requirements if you want to specify the version of flask, waitress and ploty. I'll change the Docker to use the file if that's the case? @davidusb-geek

Yes we will keep the web server requirements and erase the other requirements_addon file

Making a PR now.

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