timofurrer / w1thermsensor

A Python package and CLI tool to work with w1 temperature sensors like DS1822, DS18S20 & DS18B20 on the Raspberry Pi, Beagle Bone and other devices.
MIT License
493 stars 113 forks source link

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

Closed brettlounsbury closed 10 months ago

brettlounsbury commented 10 months ago

https://github.com/timofurrer/w1thermsensor/issues/118

brettlounsbury commented 10 months ago

Test coverage of new code is 100%.

timofurrer commented 10 months ago

@brettlounsbury awesome! 🚢

brettlounsbury commented 10 months ago

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 commented 10 months ago

Also added documentation to the Readme.

brettlounsbury commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@timofurrer - mypy issues should be fixed now.

codecov-commenter commented 10 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer) | Coverage Δ | | |---|---|---| | [src/w1thermsensor/\_\_init\_\_.py](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer#diff-c3JjL3cxdGhlcm1zZW5zb3IvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [src/w1thermsensor/async\_core.py](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer#diff-c3JjL3cxdGhlcm1zZW5zb3IvYXN5bmNfY29yZS5weQ==) | `100.00% <100.00%> (ø)` | | | [src/w1thermsensor/calibration\_data.py](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer#diff-c3JjL3cxdGhlcm1zZW5zb3IvY2FsaWJyYXRpb25fZGF0YS5weQ==) | `100.00% <100.00%> (ø)` | | | [src/w1thermsensor/core.py](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer#diff-c3JjL3cxdGhlcm1zZW5zb3IvY29yZS5weQ==) | `100.00% <100.00%> (ø)` | | | [src/w1thermsensor/errors.py](https://app.codecov.io/gh/timofurrer/w1thermsensor/pull/119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Timo+Furrer#diff-c3JjL3cxdGhlcm1zZW5zb3IvZXJyb3JzLnB5) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

timofurrer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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

brettlounsbury commented 10 months ago

@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 commented 10 months ago

@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%2Fgithub.com%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 commented 10 months ago

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