pi_ina219 icon indicating copy to clipboard operation
pi_ina219 copied to clipboard

Add type hints

Open janoskut opened this issue 3 years ago • 8 comments

It's almost 2022 - are you interested in having typehints in the code base?

Feel free to decline it, just a suggestion.

Cheers, Janos

janoskut avatar Nov 20 '21 22:11 janoskut

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.

chrisb2 avatar Nov 21 '21 02:11 chrisb2

Codecov Report

Merging #29 (27d8fae) into master (1f5da6b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

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

codecov-commenter avatar Nov 21 '21 16:11 codecov-commenter

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.

janoskut avatar Nov 21 '21 16:11 janoskut

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.

chrisb2 avatar Nov 21 '21 17:11 chrisb2

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.

janoskut avatar Nov 21 '21 18:11 janoskut

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.

chrisb2 avatar Nov 21 '21 20:11 chrisb2

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?

janoskut avatar Nov 21 '21 22:11 janoskut

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.

chrisb2 avatar Nov 21 '21 23:11 chrisb2