Fix #18

[JulianOrteil]

Adds type annotations as requested in #18.

Unfortunately, I am unable to successfully determine some types and those have been denoted with a # TODO comment. Hopefully, someone else will be able to provide the missing types via a comment and we can finish them off. Until then, all other annotations can be reviewed at leisure.

[tekktrik]

Sorry, I reviewed this a while ago and I guess just never hit "Submit review"! Attached is some feedback!

[JulianOrteil]

tekktrik, a standard practice of mine is to explicitly provide class-level attributes above __init__. I currently have a PR open in the MPR121 library providing type annotations there which shows my use of these and @FoamyGuy 's verification that microcontrollers don't appear to have any issue with them.

I use this practice as it is a typically required syntax of many languages. In python though, it makes life slightly easier for static checkers and users/contributors down the road; as well as maintaining consistency for those familiar with statically typed languages. Do you wish for me to also do that here?

And, are you able to provide the type annotations for user_data in methods like _on_subscribe_mqtt? As I note in my original comment above, I cannot determine what this parameter's type is.

[tekktrik]

If it doesn't have any effect on mpy-cross or the microcontroller I'm totally game for it! Reminds me of Cython!

And shoot, I went down the rabbit hole trying to figure it out, and I thiiink I did but didn't write it down 😭 Let me take another look.

[JulianOrteil]

Excellent. I'm currently waiting to see how the interpreter handles this syntax. I'm curious if it tosses these type declarations out like comments and docstrings, or if they are kept in. If it's the latter, I'll leave it up to you if you want me to continue since it will use more space.

[tekktrik]

Sounds good! I can compile a version without and see how mpy-cross handles it in terms of memory. My guess is that since annotations aren't saved, the lines get treated effectively like they aren't there since there's no assignment.

[tekktrik]

Update - I have no idea what type it is haha. The furthest I got was a found one library that says it's str in the do string but then found another library that uses str() which seems counterintuitive if it was already str. I'll test it out and figure it out that way.

[JulianOrteil]

Haha, no worries. If anything, we can just leave it at Optional[Any] I'd think?

I've also added the annotations for the class-level attributes. Let me know what you find and I'll use the info going forward!

[tekktrik]

Aha! Figured out where it came from! It honestly seems like a relic from when it was could be set publicly:

https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/e954938ba9b45a8f62901fb2c2c017a7f4f4976c/adafruit_minimqtt.py#L535

So yeah, Optional[Any] is the way to go!

[JulianOrteil]

Great find! Thanks for looking! I'll update this PR on Monday. Have a good weekend!

[JulianOrteil]

@tekktrik I believe this PR is ready to merge. Let me know if I missed anything and I'll update.

[tekktrik]

Nice work! A few followup comments and changes.

[tekktrik]

Hey! Just checking in on this to see how it's going? Anything I can help unblock?

[JulianOrteil]

No, sorry. I just have not been able to find time to finish this off yet. I'll hopefully get to it sometime this week.

[JulianOrteil]

@tekktrik I'll just do this now while I am thinking about this. The CI is failing because it cannot find adafruit_esp32spi and adafruit_minimqtt when building docs. Would there be any reason why I could not add these packages to the autodoc_mock_imports in conf.py?

Adding these here fixes the issue on my machine locally, but I do not know if this file is "controlled" per se like .pre-commit-config.yaml.

[tekktrik]

It's controlled, but since they're technically now dependencies, they should go in requirements.txt and setup.py so they get installed both when doing so through pip as well as for the CI when it runs. That way we can guarantee they exist at runtime for Blinka.

[JulianOrteil]

@tekktrik Everything should be ready if you want to do one final look-through.

[tekktrik]

With the dependencies updated, do you still need to mock the imports for sphinx?

[JulianOrteil]

Docs fail to build locally for me without the mock imports

[tekktrik]

Found the issue - You'll need to move all of the non-sphinx requirements out of docs/requirements.txt and into requirements.txt in order to remove all of the autodoc_mock_imports, which you can comment out as #autodoc_mock_imports = []

[tekktrik]

I forked your branch and tested it with the CI so it seems to be a local problem. What Python version are you using? Do you have all of the necessary library already installed? These looks like an issue that would occur if one of your typing imports fails.

[JulianOrteil]

Running which sphinx-build in the terminal revealed an issue I had a few months ago struck again: microsoft/vscode#142747

I use Anaconda to separate my environments since I work on many different projects. However, VSCode would sometimes prepend my base environment to $PATH and therefore would expose the python and pip binaries of my base environment first instead of the ones in the environment I have set up for this PR, for example.

When I installed Sphinx to build the docs locally, the above issue must have occurred causing sphinx to be installed in my base environment. Subsequently, because sphinx-build was installed in the wrong environment, it had no knowledge of the packages that existed in the environment I was using, hence the docs failing for me locally.

I haven't encountered the above issue since the ticket was closed, so my apologies for causing so much headache without intending to. I thought that issue was done and over with, but unfortunately, it does not appear to be so.

Anyways, installing sphinx-build correctly shows that docs build now. I wouldn't be surprised if this is also why adafruit_rsa caused problems.

[tekktrik]

I sit on a throne of lies - you will need to autodoc_mock_import = ["rtc"] since it's native to CircuitPython but not available on Blinka.

[tekktrik]

Awesome, thanks for all this!