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

Start with types #1234

Closed

Conversation

TheWizardTower
Copy link
Contributor

@TheWizardTower TheWizardTower commented Feb 29, 2020

Me, again. More stuff I want some more eyeballs on.

Most of this is in nixops/diff.py, which already had some typing work done, but I think I found a few improvements.

Namely:

  • Removed some unused imports.
  • Re-shuffled the class definitions. I can't get Handler to show up as a type Python knows about inside Handler, but if Handler comes first, then Diff methods can reference it.
  • Solved a todo that was pending python3. JKLOL nope.
  • Changed the type annotations from comments to explicit in the function defs.
  • Changed a few if conditionals to use 'not expr' or 'if exper is not None' rather than !=
  • Removed an unneeded else.

Not sure if this is at all useful, I'm just following mypy and flycheck.

Looking forward to your critical feedback. :)

nixops/diff.py Outdated
@@ -31,12 +30,10 @@ def _default_handle(self):
"""
raise NotImplementedError

def get_deps(self):
# type: () -> List[Handler]
def get_deps(self) -> List[Handler]:
return self._dependencies

def get_keys(self, *_: AnyStr) -> List[str]:
Copy link
Member

Choose a reason for hiding this comment

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

I don't even know what this code is doing :o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, me neither. :)

Copy link
Member

Choose a reason for hiding this comment

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

A handler is basically a handler of few attributes e.g handler(['region', 'instanceType']), which means it realizes the needed changes when those attributes are changed,
I think get_keys is just convenience to return the keys, don't remember why I added it to be honest :D

Copy link
Member

Choose a reason for hiding this comment

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

or are you guys asking about *_: AnyStr which I don't what it does either :-)

nixops/diff.py Outdated
def get_deps(self) -> List[Handler]:
# This is List[Handler], but that name doesn't exist inside the
# class definition.
def get_deps(self) -> List[Any]:
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be List[Handler] with from __future__ import annotations, give that a try?

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.

None yet

3 participants