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

Add type hints #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add type hints #29

wants to merge 5 commits into from

Conversation

janoskut
Copy link

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

Feel free to decline it, just a suggestion.

Cheers, Janos

@chrisb2
Copy link
Owner

chrisb2 commented Nov 21, 2021

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.

@janoskut janoskut force-pushed the add-type-hints branch 2 times, most recently from 4fcb2cd to 8754a5f Compare November 21, 2021 16:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2021

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%) ⬆️

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.

@janoskut
Copy link
Author

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.

@chrisb2
Copy link
Owner

chrisb2 commented Nov 21, 2021 via email

@janoskut
Copy link
Author

janoskut commented Nov 21, 2021

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.

@chrisb2
Copy link
Owner

chrisb2 commented Nov 21, 2021 via email

@janoskut janoskut mentioned this pull request Nov 21, 2021
@janoskut
Copy link
Author

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

@chrisb2
Copy link
Owner

chrisb2 commented Nov 21, 2021 via email

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