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

feature: Add ability to provide sensor calibration data and get corrected temperatures #119

Merged
merged 1 commit into from
Sep 16, 2023

Conversation

brettlounsbury
Copy link
Contributor

@brettlounsbury
Copy link
Contributor Author

Test coverage of new code is 100%.

@timofurrer
Copy link
Owner

@brettlounsbury awesome! 🚢

@timofurrer timofurrer self-requested a review September 12, 2023 08:17
@brettlounsbury
Copy link
Contributor Author

I just pushed an updated version that corrects the 2 formatting errors. I apologize for that - tox -e lint was reformatting all of the files and I didn't want to push a change that contained massive amounts of reformatting across the whole project. I guess I missed a spot :-).

@brettlounsbury
Copy link
Contributor Author

Also added documentation to the Readme.

@brettlounsbury
Copy link
Contributor Author

@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!

@timofurrer
Copy link
Owner

@brettlounsbury thanks for the update, it's still failing with mypy though. I think you can run the typing tox env locally to verify.

@brettlounsbury
Copy link
Contributor Author

@timofurrer - mypy issues should be fixed now.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4166935) 100.00% compared to head (2451b47) 100.00%.
Report is 1 commits behind head on master.

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     
Files Changed Coverage Δ
src/w1thermsensor/__init__.py 100.00% <100.00%> (ø)
src/w1thermsensor/async_core.py 100.00% <100.00%> (ø)
src/w1thermsensor/calibration_data.py 100.00% <100.00%> (ø)
src/w1thermsensor/core.py 100.00% <100.00%> (ø)
src/w1thermsensor/errors.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timofurrer
Copy link
Owner

Test coverage of new code is 100%.

@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 :)

@brettlounsbury
Copy link
Contributor Author

Hmm, I guess by new code i meant calibration_data.py.

I added some additional tests to check the error handling in core.py and async_core.py where calibration_data.py is not provided.

Sorry for all the back and forth :-)

@brettlounsbury
Copy link
Contributor Author

@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 :-)

@brettlounsbury
Copy link
Contributor Author

Found out what the problem with the coverage on async-core was. I had placed the await on construction of the sensor instead of reading the sensor in the test code... woops.

@brettlounsbury
Copy link
Contributor Author

@timofurrer - it should be good now. Apologies again for the back/forth.

@brettlounsbury
Copy link
Contributor Author

@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?

@brettlounsbury
Copy link
Contributor Author

@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
https://codecov.io/upload/v4?package=github-action-v1.5.2-1.0.3&token=secret&branch=master&commit=2451b47fd7e9733bcfbfe81a02de3c08754fedc6&build=6191871537&build_url=http%3A%2F%2Fgit.luolix.top%2Ftimofurrer%2Fw1thermsensor%2Factions%2Fruns%2F6191871537&name=&tag=&slug=timofurrer%2Fw1thermsensor&service=github-actions&flags=&pr=119&job=CI&cmd_args=n,F,Q,Z,C
{'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

@timofurrer timofurrer merged commit 88bbcea into timofurrer:master Sep 16, 2023
7 checks passed
@timofurrer
Copy link
Owner

@brettlounsbury re-triggering helped! Thanks for all the good work - I've just merged it 🎉

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.

3 participants