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

configuration page init pv switch #407

Conversation

GeoDerp
Copy link
Contributor

@GeoDerp GeoDerp commented Dec 29, 2024

No description provided.

@GeoDerp
Copy link
Contributor Author

GeoDerp commented Dec 29, 2024

@davidusb-geek , My biggest concern is that his will likley be a braking change for current users who use PV (since the default is false ). Im thinking now for a possible solution.

We could hardcode in a function that checks if pv_module_model or pv_inverter_model has been defined. (dont equal the config_defaults.json default values) If true, set pv to true and save.

@GeoDerp GeoDerp marked this pull request as ready for review December 29, 2024 01:18
@GeoDerp
Copy link
Contributor Author

GeoDerp commented Dec 29, 2024

Last commit is a possible idea arround the issue. It still passes get_weather_forecast if the user passes data via runtime. (thanks to @mangonel for the idea)

@davidusb-geek
Copy link
Owner

@davidusb-geek , My biggest concern is that his will likley be a braking change for current users who use PV (since the default is false ). Im thinking now for a possible solution.

We could hardcode in a function that checks if pv_module_model or pv_inverter_model has been defined. (dont equal the config_defaults.json default values) If true, set pv to true and save.

Yes this will be a breaking change but we will be transparent about it and the fix will just need to turn on that new switch.
The new hardcoded function will be the elegant solution. If you can spend some time on that just tell me, otherwise I will release with the explicit breaking change

@davidusb-geek davidusb-geek merged commit 9ff95f0 into davidusb-geek:davidusb-geek/dev/pv_install_optional Dec 29, 2024
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.

2 participants