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.

Sign up for GitHub

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

Jump to bottom

Start with types rebase #1235

Merged
merged 18 commits into from Feb 29, 2020
Merged

Start with types rebase #1235

merged 18 commits into from Feb 29, 2020

Conversation

TheWizardTower
Copy link
Contributor

Let's try this again.

I think I vaporized my other branch, which nullified my existing PR. But. This is the same code.

@TheWizardTower
Remove unused imports, sort typing import, import locals last.
ac65c21
@TheWizardTower
Move Handler class to top of file, remove object imports.
b462d8c
@TheWizardTower
Do the todo! also change the type annotation style.
866defe
@TheWizardTower
Change the type signature style, make it explicitly return List[Handler]
0c48998
@TheWizardTower
Polish if-conditional style. Pacify the linter.
eef7958
@TheWizardTower
Define this in the init, to remove a lint in the set_handlers function.
4630e69
@TheWizardTower
Remove unneeded else.
a011e24
@TheWizardTower
CI bit me, this does not work, fix and re-try.
d4eea14
@TheWizardTower
Remove duplicate type signatures, thank you CI!
103ea8e
@TheWizardTower
Set the default value in a saner way, add type annotation.
9ee1912
@TheWizardTower
Add typing.Tuple, define h_tuple type, define keys type.
41ddbfb
@TheWizardTower
Bah, this slipped past me. Remove a crashing type annotation.
7f8d96e
@TheWizardTower
Remove unused imports, sort typing import, import locals last.
c346572
@TheWizardTower
CI bit me, this does not work, fix and re-try.
396c025
@TheWizardTower
This seems to work?
0549ff0
@TheWizardTower
CI complained that the __future__ import must be at top of file. Fix.
604178c
@grahamc
Copy link
Member

grahamc commented Feb 29, 2020

I wonder if that future patch makes the nixops.deployment.Deployment type hint work, too. I've wanted to do that elsewhere but got bit by the circular dependency.

grahamc
grahamc approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

lgtm!

Want to try that __future__ annotations thing for the Deployment type in this one? I'm happy to merge as-is.

@TheWizardTower
Copy link
Contributor Author

It looks like it passes mypy on my machine? No harm in giving it a go here.

@grahamc
Copy link
Member

grahamc commented Feb 29, 2020

Blah. Dang. Burned by reality. Let's undo that and merge :)

@TheWizardTower
Copy link
Contributor Author 

ImportError: cannot import name 'Deployment' from 'nixops.deployment' (/build/nixops-1.8pre0_abcdef/nixops/deployment.py)

I think this is toast. :(

will un-merge.

@grahamc grahamc merged commit f691fd5 into NixOS:start-with-types Feb 29, 2020
@grahamc
Copy link
Member

grahamc commented Feb 29, 2020

Thank you, this is amazing!

@grahamc grahamc added this to the 2.0 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@TheWizardTower @grahamc