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 mypy type annotations via monkeytype #344

Merged

Conversation

gnmerritt
Copy link
Contributor

@gnmerritt gnmerritt commented Sep 8, 2022

Just throwing this up here so I can let the tests run against the PR while I keep working, and so that you can take a look at where things are headed if you're curious


so far I've been able to get MonkeyType to produce decent annotations for

BAC0.tasks.RecurringTask
BAC0.core.devices.Points
BAC0.core.io.Read
BAC0.tasks.Poll
BAC0.core.functions.GetIPAddr
BAC0.core.functions.TimeSync
BAC0.core.proprietary_objects.object
BAC0.core.app.ScriptApplication

I was hoping to get better coverage, but monkey type isn't good about picking up annotations across threads and with the TaskManager as the main point of a lot of the functionality I wasn't able to get great annotations automatically. The good news is what I was able to get should be enough to start bootstrapping a mypy run that catches "obviously wrong" bugs like missing function arguments or badly typed string.format calls.

#closes #324

@ChristianTremblay
Copy link
Owner

Looks like a lot of work. I'm not used to typing a lot. Anything I should know of your workflow ?

@gnmerritt
Copy link
Contributor Author

In the ideal case it's minimal extra work - I've set it up so you can skip most annotations with the caveat that anywhere the type checker can't figure things out it's less able to help catch errors. It'll run the checks right after flake8 as part of the github actions.

On your local machine there are a lot of ways to run the checker - easiest is to install the mypy-requirements.txt file and then run mypy from the command line. I'd recommend however that you set up your text editor to give you flake8 and mypy errors after each save, so that you can see them inline while you're working. It's also nice (for functions that are annotated) to be able to see argument and return types inline in the editor.

One last explicit callout: even without many annotations mypy has caught a couple runtime errors in format strings. They're especially nasty because a couple are in debug logging in catch blocks so I can't imagine they run very often...

@gnmerritt gnmerritt changed the title WIP: Add mypy type annotations via monkeytype Add mypy type annotations via monkeytype Sep 12, 2022
@gnmerritt
Copy link
Contributor Author

gnmerritt commented Sep 12, 2022

okay @ChristianTremblay I've got type checks passing for a subset of the codebase and it looks like the unit tests are working too.

I'm going to deploy this at a building tomorrow and let it bake for a couple days there and if you have a moment I think this should also be ready for any code-review you want to do. I'll take a pass through it myself too and make sure things look okay on my end.

I tried to leave all existing logic intact, except in cases where it was clear that I could make the code accept more input values than it currently does.
I think one place the type hints are still not right is on the return type of ReadProperty.read, so if you have suggestions there (or anywhere else!) I'm happy to take another pass at it.

And please let me know if you have any questions about the type annotations themselves!

Copy link
Contributor Author

@gnmerritt gnmerritt left a comment

Choose a reason for hiding this comment

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

okay, things look good as far as I'm concerned. I tried to call out the reasons for most of the non-annotation code changes both for posterity and to convince myself they were all necessary

BAC0/core/devices/Device.py Show resolved Hide resolved
@@ -734,7 +724,12 @@ def __repr__(self):
)
)
# Probably disconnected
val = None
return "{}/{} : (n/a) {}".format(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to duplicate the string formatting here because it's an error to use {:.2f} on None

@@ -1187,8 +1183,8 @@ def __init__(self, device, name):

self.properties.description = props["description"]
self.properties.units_state = props["units_state"]
self.properties.simulated = "Offline"
self.properties.overridden = "Offline"
self.properties.simulated = (True, None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked and didn't see anywhere checking for these strings, and they seem to conflict with the usual type of Tuple[bool, Optional[int]]

BAC0/core/devices/Trends.py Show resolved Hide resolved
BAC0/core/functions/Calendar.py Show resolved Hide resolved
BAC0/core/io/Read.py Show resolved Hide resolved
BAC0/db/sql.py Show resolved Hide resolved
BAC0/scripts/Complete.py Show resolved Hide resolved
BAC0/scripts/Lite.py Show resolved Hide resolved
BAC0/tasks/Poll.py Show resolved Hide resolved
@gnmerritt
Copy link
Contributor Author

@ChristianTremblay thanks for taking a look! how are you feeling about merging this? is there anything else you'd like me to do infrastructure or documentation wise?

@ChristianTremblay
Copy link
Owner

My plan is to tag a new release from actual develop.
Then we'll merge.

@ChristianTremblay ChristianTremblay merged commit 96dcef2 into ChristianTremblay:develop Sep 22, 2022
@ChristianTremblay
Copy link
Owner

Thanks ! Let's dive into that now !

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