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
Heating scheduler #37
Conversation
firstdayofjune
commented
Feb 18, 2021
- Added a client to communicate with the IOLITE heating API (plus tests)
- Added some helpers to the Discovered
- Added a pyproject.toml file to configure isort & black
- Minor refactorings to code and test
- Readme updated
- added the iolite directory as an editable dev-dependency, so iolite can be imported without being installed (and without a need to manipulate the path) - changed the environment-accesses to use environs instead of os.getenv
- added black and isort as dev-dependencies to use them in file watchers - as all black releases are pre-releases, a specific version number is required to avoid an error when locking (I chose the latest version)
- added black and isort as dev-dependencies to use them in file watchers - chose isort 4.3.21 to match the pre-commit setup - chose black 20.8.b to match the pre-commit setup - also all black releases are pre-releases, a specific version number is required to avoid an error when locking - added a pyproject.toml to configure isort
- moved place and device response handling into separate methods
- added toml as a pre-commit dependency - added toml to pipenv
- added a Heating Scheduler object for interaction with the heating intervals endpoint - added helpers to get rooms and entities to the Discovered class, to use these rooms with the Heating Scheduler - added a jupyter notebook to showcase the Heating Scheduler - added tests and extended client tests, for safer refactoring - updated the readme to introduce the Heating Scheduler
# Conflicts: # Pipfile.lock
- updated lock file after merge
- updated lock file afterwards
- added missing spaces to bulletpoints in README.md - suppressed security warning in test_heating_scheduler.py, as the password is hard-coded deliberately and only used to verify that the client encodes a given password correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments but looking good 👍🏼
@@ -10,6 +10,11 @@ pre-commit = "*" | |||
coverage = "*" | |||
pytest-cov = "*" | |||
codacy-coverage = "*" | |||
iolite = {editable = true,path = "."} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to declare this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the example.py threw an error because iolite was not an installed package. So I added the iolite directory as an editable dev-dependency. This way you can import iolite
without having the package actually installed in the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh now I see it. It feels weird having the package add itself as a dependency but I am not sure of a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another solution would be to turn scripts
into a module (add __init__.py
) and then it can be invoked as python -m scripts.example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work for the scripts, yes. I just realized the iolite = {editable = true,path = "."}
is also needed to import iolite inside the notebooks, though. So I would like to keep this change.
@firstdayofjune sorry for the conflict in the Pipfile - shouldn't be hard to fix and regenerate the I also fixed the trigger for CI to work against your branch so if you merge/rebase against master that should work now 👍🏼 |
- renamed discovered into discovered_rooms, as only rooms will be stored - refactored Interval min/max temperatures into constatnts - detailed the instructions on how to install jupyter notebook and how to fix a ModuleNotFoundError that may occur when using jupyter from within the venv
- Changed black version to 19.10b to match pipenv-setup's dependency - Regenerated lock file # Conflicts: # Pipfile # Pipfile.lock
Applied all the changes as discussed. It took me some time to recreate the lockfile until I found out that pipenv-setup requires black at version ==19.10b0, when I had ==20.8b1 in dev packages 🤦♀️ |
Something must be up with CI - it's happening with other PR's #46 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the pipeline that should only upload coverage on master now.
Going forward I think we should swap out Codacy for coverage and use an alternative platform.
pytest-socket = "*" | ||
freezegun = "*" | ||
|
||
[packages] | ||
websockets = "*" | ||
environs = "*" | ||
requests = "*" | ||
|
||
[pipenv] | ||
allow_prereleases = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this declared? Do we really need pre-release dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black is only available as a pre-release package and pipenv does not install pre-releases without actively allowing it to do so. When installing it with pipenv install black --dev --pre
the "allow_prereleases" part is automatically added by pipenv.
Once black becomes available as a major release, this can be removed again. I guess this can take some time, though: psf/black#1746
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the need for black
? I've set it up as a pre-commit hook which you can install via pre-commit install
or just run manually with pre-commit run --all
?
Which is also called on CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having black, isort, and flake8 installed in the environment, I use file watchers to run them on every file save. I prefer this setup to running the pre-commit hook once in a while, because I immediately see the changes made by black and isort. Also, this way I can fix the flake8 warnings right away.
I could probably also just run the pre-commit hook, this feels slightly slower, though (and runs on all files, not just the file I am working on).
So I added the scripts as dev dependencies, cause I though it does no harm 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okays - that makes sense.
I think pre-commit runs on the diff by default but you can also tell it to run on all changes.
We can always remove the dev dependencies if it becomes too much.
@firstdayofjune fixed CI 👍🏼 |
Thanks 👍🏼 |