-
-
Notifications
You must be signed in to change notification settings - Fork 151
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 to tasks.py #241
Conversation
f603667
to
13bf2a0
Compare
This is great, but we should try to find a way to remove the circular dependency without splatting t he whole manifest module into sessions. |
Indeed. I'd sleep much easier if there was a PR dedicated to resolving the circular dependency totally distinct from this one. @cs01 Generating type hints is very much appreciated! |
Sounds good, I will do that in a different PR and then revisit this one. |
Oh something else that came up that was kind of annoying was the import sorting errors. I ran |
honestly we can just disable flake8's import order checking.
…On Mon, Aug 26, 2019 at 10:48 AM Chad Smith ***@***.***> wrote:
Sounds good, I will do that in a different PR and then revisit this one.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#241>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB5I42QFDFJOZD664IDKZLQGQJOLANCNFSM4IPJUDLA>
.
|
13bf2a0
to
0bcedad
Compare
I created a new PR to update import order linting in #242. This PR depends on that one, so it should be reviewed first. This PR has been reduced in scope to only add types to |
Merged master into this PR. Now the diff is much smaller and only affects types in tasks.py. |
Rad, thanks! |
Generate type hints database from types used during runtime:
See modules that can have types applied:
Apply types
This exposed a circular dependency between session.py and manifest.py which was resolved in this PR. It ended up causing many LOC to be changed, but the change was cut+pasting code.
There are still several modules that haven't had types added yet since this PR got quite large.
Note that there are no type errors, unit tests did not change, and test coverage is still 100%.