Open janoskut opened 3 years ago
This looks interesting, I will do some reading. I am currently working on replacing Adafruit GPIO (archived) with smbus2 in v2.0.0 of library, so this seems like a good time to introduce this.
Merging #29 (27d8fae) into master (1f5da6b) will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #29 +/- ##
=======================================
Coverage 99.14% 99.14%
=======================================
Files 1 1
Lines 234 235 +1
Branches 20 18 -2
=======================================
+ Hits 232 233 +1
Misses 1 1
Partials 1 1
Impacted Files | Coverage Δ | |
---|---|---|
ina219.py | 99.14% <100.00%> (+<0.01%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 1f5da6b...27d8fae. Read the comment docs.
I'm realizing that adding type hints breaks compatibility with Python 3.5 and 2.7. There are ways for type-hinting Python 2 code as well (see here), but I'm not sure if it's worth it then.
I have removed 2.7 and 3.5 from my convert-to-smbus2 branch so no need to deal with these.
Chris
On Mon, 22 Nov 2021, 05:38 Janos Kutscherauer, @.***> wrote:
I'm realizing that adding type hints breaks compatibility with Python 3.5 and 2.7. There are ways for type-hinting Python 2 code as well (see here https://mypy.readthedocs.io/en/latest/python2.html#type-checking-python-2-code), but I'm not sure if it's worth it then.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chrisb2/pi_ina219/pull/29#issuecomment-974851179, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXR2V4342QAE72YC7YE2MDUNEN6VANCNFSM5IONEN3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
I have removed 2.7 and 3.5 from my convert-to-smbus2 branch so no need to deal with these.
Awesome!
Regarding this smbus2-branch - do you mind to further abstract the whole I2C access out, to have the user choose what driver they're using? I for example am currently using smbus
in a project (I don't know why, actually), and the first thing I was doing was to rewrite that part. I'll suggest something in another PR in a bit.
Yes smbus and smbus2 will be interchangeable. I choose smbus2 as smbus looks like a dead repo with no recent maintenance. What other drivers are you thinking of?
Any way I agree this seems reasonable.
Chris
On Mon, 22 Nov 2021, 07:50 Janos Kutscherauer, @.***> wrote:
I have removed 2.7 and 3.5 from my convert-to-smbus2 branch so no need to deal with these.
Awesome!
Regarding this branch - do you mind to further abstract the whole I2C access out, to have the user choose what driver they're using? I for example am currently using smbus in a project (I don't know why, actually), and the first thing I was doing was to rewrite that part. I'll suggest something in another PR in a bit.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chrisb2/pi_ina219/pull/29#issuecomment-974872289, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXR2V6DDZOEZYTSUST2MVTUNE5N5ANCNFSM5IONEN3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
What other drivers are you thinking of?
I wouldn't know of any others myself, but searching for it, there seem to be some others. See this PR for my suggestion: https://github.com/chrisb2/pi_ina219/pull/30. I'm not sure if everything needs to be abstracted... It makes the usage slightly more complicated, but gives the freedom of tool on the other hand.
For >this< PR - I don't seem to get Travis to pass. It seems to build older commits and is still failing on 2.7/3.5, although I removed them. Could you assist in getting it to pass?
Will take a look later , at work atm
Chris
On Mon, 22 Nov 2021, 11:55 Janos Kutscherauer, @.***> wrote:
What other drivers are you thinking of?
I wouldn't know of any others myself, but searching for it, there seem to be some others. See this PR for my suggestion: #30 https://github.com/chrisb2/pi_ina219/pull/30. I'm not sure if everything needs to be abstracted... It makes the usage slightly more complicated, but gives the freedom of tool on the other hand.
For >this< PR - I don't seem to get Travis to pass. It seems to build older commits and is still failing on 2.7/3.5, although I removed them. Could you assist in getting it to pass?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chrisb2/pi_ina219/pull/29#issuecomment-974912880, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXR2V46ZWVVQ63TWW7FDFLUNF2E5ANCNFSM5IONEN3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
It's almost 2022 - are you interested in having typehints in the code base?
Feel free to decline it, just a suggestion.
Cheers, Janos