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

Support reading of multiple values at once #56

Merged
merged 9 commits into from
Jan 15, 2023

Conversation

golles
Copy link
Owner

@golles golles commented Jan 11, 2023

  • Move kamstrup.py to its own module
  • Refactor kamstrup.py and add typings

@adabrandt for your code suggestion in bsdphk/PyKamstrup#6

-Move `kamstrup.py` to it's own module
-Refactor `kamstrup.py` and add typings
@golles golles linked an issue Jan 11, 2023 that may be closed by this pull request
1 task
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Base: 16.89% // Head: 73.86% // Increases project coverage by +56.96% 🎉

Coverage data is based on head (bfbe837) compared to base (cbc618f).
Patch coverage: 52.41% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #56       +/-   ##
===========================================
+ Coverage   16.89%   73.86%   +56.96%     
===========================================
  Files           6        7        +1     
  Lines         290      329       +39     
===========================================
+ Hits           49      243      +194     
+ Misses        241       86      -155     
Impacted Files Coverage Δ
custom_components/kamstrup_403/__init__.py 85.00% <50.00%> (+55.73%) ⬆️
...tom_components/kamstrup_403/pykamstrup/kamstrup.py 51.16% <51.16%> (ø)
custom_components/kamstrup_403/config_flow.py 95.74% <100.00%> (+95.74%) ⬆️
custom_components/kamstrup_403/pykamstrup/const.py 100.00% <100.00%> (ø)
custom_components/kamstrup_403/sensor.py 94.11% <0.00%> (+94.11%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@StevenLooman
Copy link

Great work! You can consider moving kampstrup.py to its own package and publish it to PyPI even. This way others can use it even more easily. It also keeps the home assistant parts (this repo) and the kampstrup interfacing separate.

Also, parentheses are only needed when catching multiple exception types. E.g., you can omit the parentheses on this line:
https://github.com/golles/ha-kamstrup_403/pull/56/files#diff-b42daff407aa09858d366cd76d1042a12e82de32c92ea02115b6e6dd8393bae1R141

@golles
Copy link
Owner Author

golles commented Jan 11, 2023

Great work! You can consider moving kampstrup.py to its own package and publish it to PyPI even. This way others can use it even more easily. It also keeps the home assistant parts (this repo) and the kampstrup interfacing separate.

I have thought about it, and I'm not entirely sure if I want to take public ownership of it, also because it's not really my code. But with the change I made in this PR, I agree it's pretty standalone. I'll give it another thought...

Also, parentheses are only needed when catching multiple exception types. E.g., you can omit the parentheses on this line: https://github.com/golles/ha-kamstrup_403/pull/56/files#diff-b42daff407aa09858d366cd76d1042a12e82de32c92ea02115b6e6dd8393bae1R141

Thanks, I found a few more as well, fixed in 8192f0d

@StevenLooman
Copy link

I have thought about it, and I'm not entirely sure if I want to take public ownership of it, also because it's not really my code. But with the change I made in this PR, I agree it's pretty standalone. I'll give it another thought...

Perhaps asking @bsdphk directly helps, maybe he can give his opinion?

@golles golles merged commit 2b35f99 into main Jan 15, 2023
@golles golles deleted the 29-support-reading-of-multiple-values-at-once branch January 15, 2023 14:21
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.

Support reading of multiple values at once.
2 participants