Skip to content
This repository was archived by the owner on May 16, 2023. It is now read-only.

Add Missing Type Annotations #28

Merged
merged 10 commits into from
Nov 11, 2022
Merged

Add Missing Type Annotations #28

merged 10 commits into from
Nov 11, 2022

Conversation

tcfranks
Copy link
Contributor

resolves #25
submitted for review / comment

Note: in thermal_printer_2168.py the function warm_up accepts an argument "heat_time" and never makes use of it...should it?

@tcfranks
Copy link
Contributor Author

@tekktrik , @FoamyGuy - um, local pylint disagrees with the automated run or I wouldn't have submitted - can you interpret the error for me?

@tekktrik
Copy link
Member

It's a warning about code duplication - it's a pylint warning describing that code is extremely similar and could probably be refactored. I don't think that's necessary here so we should just disable the warning. I'll get back to you on how to do so shortly, or may just push the change myself if that's okay, let me know

@tcfranks
Copy link
Contributor Author

either way is fine.....I was able to get local pylint (run on entire directory) to duplicate the error but not suppress it. That was a learning point for me on running on multiple files. but evidently, via quick Google, "# pylint: disable=duplicate-code" isn't something that is (easily?) implemented, so, whatever you think is best....

@tekktrik
Copy link
Member

The trick is to disable it either in .pylintrc (which disables it for pylint) or in .pre-commit-config.yaml (which disables it for the pre-commit hook of pylint). I think the latter is preferable because 1) it won't get lost in the other disables if this ever does get refactored and B) it seems like something we'd rather silence in pre-commit as opposed to pylint directly. It's how we've done this in the past. You can see that last commit for how I applied the fix!

@tekktrik
Copy link
Member

Note: in thermal_printer_2168.py the function warm_up accepts an argument "heat_time" and never makes use of it...should it?

No, let's keep it - I think it's worth preserving this change with the scope of this PR. We make API-breaking changes in another one.

@tcfranks
Copy link
Contributor Author

resubmitted for review with reviewer requested changes

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback below

@tcfranks
Copy link
Contributor Author

resubmitted for review with reviewer comments

@tcfranks
Copy link
Contributor Author

resubmitted with 1 change

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tekktrik tekktrik merged commit db73533 into adafruit:main Nov 11, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Nov 12, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_Thermal_Printer to 1.3.12 from 1.3.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_Thermal_Printer#28 from tcfranks/main
  > Merge pull request adafruit/Adafruit_CircuitPython_Thermal_Printer#31 from tekktrik/dev/fix-pylint-errors
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Type Annotations
3 participants