Skip to content

Added requested type annotations to epd.py. #65

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

Merged
merged 27 commits into from
May 22, 2023

Conversation

sdomoszlai13
Copy link
Contributor

Please let me know if something's wrong with the annotations.

@sdomoszlai13
Copy link
Contributor Author

sdomoszlai13 commented May 9, 2023

The Build CI test fails because of a pylint error, which should be ignored if I'm not mistaken (there's the '# pylint: disable=too-many-arguments' directive in the code in those lines). Can you tell me what's wrong with my commit?

Also, excuse me for closing the other pull request. I just meant to rename that branch to a better name.
@FoamyGuy

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

@sdomoszlai13 thanks for working on this! I have some suggestion changes noted below in a few sections of the code.

I would also say it would be easiest to review if you can merge or copy in the changes from the other PRs that are typing out the separate driver class files in the library. If you have a strong preference to keep them separate that is okay as well, but combining everything into this one will make it a bit easier.

@@ -176,7 +192,7 @@ def command(self, cmd, data=None, end=True):

return ret

def _spi_transfer(self, data):
def _spi_transfer(self, data: Any) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that data can be Union[int, bytearray] and the return type can be int

@FoamyGuy
Copy link
Contributor

with regards to the pylint error. You can add too-many-arguments to the disabled checks on the class declaration line:

It's already got a few different ones but this one can be added at the end of the list like this:

class Adafruit_EPD:  # pylint: disable=too-many-instance-attributes, too-many-public-methods, too-many-arguments

With it disabled from there it won't need to be included on any of the individual functions and it will allow it to pass the check

@sdomoszlai13
Copy link
Contributor Author

sdomoszlai13 commented May 18, 2023

Thanks for your valuable suggestions @FoamyGuy ! I implemented them in this branch and merged the others in this one. Now you can review my changes made in the other files here as well.

Locally, the pylint test completes without errors, but in GitHub it complains about the type Image, which is part of the PIL package:

File "/home/runner/work/Adafruit_CircuitPython_EPD/Adafruit_CircuitPython_EPD/adafruit_epd/epd.py", line 399, in Adafruit_EPD
def image(self, image: Image) -> None:
^^^^^
NameError: name 'Image' is not defined

I have the import command inside the try block:

from PIL.Image import Image

Any suggestions?

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I have a few more small tweaks to suggest. But this is looking very close to good to me at this point.

Thanks again for all of your work on this @sdomoszlai13

@sdomoszlai13
Copy link
Contributor Author

sdomoszlai13 commented May 20, 2023

Requested changes implemented. Thanks for your support @FoamyGuy !
Should I close the other pull requests, since I merged the corresponding branches to this branch?

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

The latest version of this is looking good to me.

I tested it successfully on a Mag Tag by releasing the built-in display and initializing it with this driver library.

I did commit one tweak: 148c744 to move the Direction import back out of the try block. That one we do need to import on the microcontroller at runtime because we use some constants from it for setting pin configurations. But I did notice that pylint complains about that because the digitalio imports are ungrouped. I've added the flag for pylint to ignore that error in this file because they are ungrouped purposefully to avoid importing the one that isn't needed in order to save RAM.

@FoamyGuy FoamyGuy merged commit df01018 into adafruit:main May 22, 2023
@FoamyGuy
Copy link
Contributor

@sdomoszlai13 Thanks again for working on this!

Yes you can close the other PRs now since everything is included in this one.

@FoamyGuy
Copy link
Contributor

Oh, I noticed after I left the prior comment that Github seems to have figured it out automatically and closed those other ones so you shouldn't have to do anything on your end actually.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 23, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.11.2 from 2.11.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#65 from sdomoszlai13/fix-annotations-epd.py

Updating https://github.com/adafruit/Adafruit_CircuitPython_asyncio to 0.5.21 from 0.5.20:
  > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#41 from Neradoc/fix-package-prefix

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 4.0.0 from 3.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#54 from michalpokusa/4.0.0-examples-refactor-authentication-mimetypes
  > Run pre-commit
  > Update pre-commit hooks
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#53 from adafruit/not-robust

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@FoamyGuy FoamyGuy mentioned this pull request Jun 7, 2023
66 tasks
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.

2 participants