-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feature: Add ability to provide sensor calibration data and get corrected temperatures #119
Conversation
Test coverage of new code is 100%. |
@brettlounsbury awesome! 🚢 |
d7bbdb9
to
77c5e41
Compare
I just pushed an updated version that corrects the 2 formatting errors. I apologize for that - |
77c5e41
to
8a400c0
Compare
Also added documentation to the Readme. |
@timofurrer - I've re-requested a review since I changed the code slightly (to make flake pass), and to add Readme details. Do you mind reviewing and then running the workflow and merging if it looks good to you? Thanks! |
@brettlounsbury thanks for the update, it's still failing with mypy though. I think you can run the |
8a400c0
to
c823ff8
Compare
@timofurrer - mypy issues should be fixed now. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #119 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 9 +1
Lines 316 365 +49
Branches 86 97 +11
=========================================
+ Hits 316 365 +49
☔ View full report in Codecov by Sentry. |
@brettlounsbury apparently it's not, but I'm okay with that. I'll give you some time in case you want the 100% - otherwise I'm going to merge as-is later :) |
c823ff8
to
760dce8
Compare
Hmm, I guess by I added some additional tests to check the error handling in Sorry for all the back and forth :-) |
@timofurrer - this should be ready for you to run the workflow and pull at this point. I added some extra tests to cover the error conditions that I missed, and otherwise I think it seems to be in a good state :-) |
760dce8
to
ec48949
Compare
Found out what the problem with the coverage on async-core was. I had placed the |
@timofurrer - it should be good now. Apologies again for the back/forth. |
…cted temperatures.
ec48949
to
2451b47
Compare
@timofurrer - whenever you can, would you mind re-running the workflow so I can verify the fixes I made to the tests resulted in increased coverage? |
@timofurrer - im looking at the CI jobs, and the coverage report shows 100% coverage and tests pass, but it looks like they fail to upload the coverage report to CodeCov. Not sure if thats a known issue or not - it hasn't happened previously during this PR. -> Pinging Codecov |
@brettlounsbury re-triggering helped! Thanks for all the good work - I've just merged it 🎉 |
#118