search

davidusb-geek / emhass

emhass: Energy Management for Home Assistant, is a Python module designed to optimize your home energy interfacing with Home Assistant.
MIT License
260 stars 51 forks source link

Alternative method to Utils optional options #169

Closed GeoDerp closed 5 months ago

GeoDerp commented 5 months ago

@davidusb-geek You can pick one or the other. (other being #167) Personally I like this approach more as its easy to read. However mat be complex to understand.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6fe81cf) 88.54% compared to head (12f1be5) 89.96%.

Files Patch % Lines
src/emhass/forecast.py 33.33% 2 Missing :warning:
src/emhass/utils.py 98.59% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #169 +/- ## ========================================== + Coverage 88.54% 89.96% +1.41% ========================================== Files 6 6 Lines 1668 1644 -24 ========================================== + Hits 1477 1479 +2 + Misses 191 165 -26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidusb-geek commented 5 months ago

This option seems much more elegant. Nice work.

davidusb-geek commented 5 months ago

Both solar.forcast and solcast should be broken. That was the main reason for both pull requests initially. Would have to check if that's related to that comment however.

The bugs from this comment are also solved on this PR. In other words, if we use this PR instead of #167, the bugs on solar.forcast and solcast are solved?

GeoDerp commented 5 months ago

In testing it seemed to fix the issue also. Will do a double check now to be double sure. (But yeah if you pull this one we will close the other)

davidusb-geek commented 5 months ago

I think that we will push up this method, if everything works correctly.

GeoDerp commented 5 months ago

Tested, looks like everything works. Actually found an bug when setting solar_forecast_kwp to 0 so added a patch fix (feel free to revert once pulled)

GeoDerp commented 5 months ago

hangon. may of found another bug. Looking at it now. Having an issue with new_string = string.replace(var_load, var_load+'_positive') when I delete all parameters in options.json Trying to work out why.

Edit: I think I know why

GeoDerp commented 5 months ago

@davidusb-geek Should be good to go now 👍 My most tested code yet (not saying its perfect)

davidusb-geek commented 5 months ago

Great! +1