migra icon indicating copy to clipboard operation
migra copied to clipboard

PEP 484/Mypy type annotations?

Open SKalt opened this issue 4 years ago • 5 comments

Would you be interested in using mypy to validate PEP 484 type annotations as a part of migra's CI? If so, what syntax would you find acceptable? There's:

  1. My first choice, python 3.5+ native syntax:
from typing import Union
def foo(a: str, b: int) -> Union[int, str]:
    return b if b else a
  1. the other option, python 2.7+ inline comments
def foo(
    a, # type: str
    b # type: int
):
     return b if b else a
  1. shipping .pyi stub files, which can't be used to check the migra codebase.

I'd be willing to take this on over the weekend.

SKalt avatar Jan 16 '20 15:01 SKalt

Hey, this would be awesome! I've been meaning to get these added to the codebase for a while now.

Agree with the preference for the most modern syntax.

Worth noting that while the codebase supports python 2 at the moment, I've been planning to bite the bullet, stop supporting it, clean up the codebase, and take advantage of some 3-only features like dataclasses and natively ordered dictionaries. So don't worry about supporting old versions.

Excited to see how the code looks with annotations added!

djrobstep avatar Jan 16 '20 22:01 djrobstep

Question barrage incoming. I've been able to narrow down the type errors over on my branch [linked], but I'm unable to figure out the following:

  • What are the arguments does sqlbag.get_inspector(x, schema=None), specifically in its callback to connection_from_s_or_c? Is x supposed to be a sqlalchemy.Connection or sqlalchemy.Engine?
  • What are the argument types of statements_from_differences and get_table_changes?
  • What about the argument types of inner functions has_remaining_dependents and has_uncreated_dependencies?

SKalt avatar Jan 20 '20 20:01 SKalt

Cheers for raising all these issues, lots of room for improvement here. Things are a bit hectic for me over the next few days so won't have time to address all these issues immediately, but a few quick ones:

  • get_inspector can accept an sqlalchemy session or connection (possibly an engine too, can't remember off the top of my head)
  • statements_from_differences takes dictionaries of things (qualified_identifier -> inspected[whatever]) that have been added, removed, modified etc, some booleans, and replaceable which is a set of identifiers of things (views/functions) that can be safely replaced, and old which is the things_from from statements_for_changes
  • has_remaining_deps and has_uncreated_deps take the inspected[whatever] object as a first parameter, and a set of qualified identifiers (names) as the second.

djrobstep avatar Jan 21 '20 23:01 djrobstep

update: I'm currently stuck working adding type annotations to the stuff that reqires sqlalchemy internals. Turns out typeshed does not include sqlalchemy stubs. Instead, there are potentially-usable stubs at https://github.com/dropbox/sqlalchemy-stubs, which is in alpha.

Once again, I should have time to make progress this weekend.

SKalt avatar Jan 24 '20 14:01 SKalt

I managed to eliminate type errors within migra, but making stubs for sqlbag and schemainspect hasn't got me far. It will likely be easier to add typing to those modules themselves.

Also, I've hit a wall trying to type callbacks. I had to nuke the great metaprogramming in changes.py; I'd like to write something like

# clear pseudocode

class Callback(Protocol): # use a callback protocol as the method type
    def __call__(self, specific: typed = defaulted, keyword: ars = defaulted) -> Statements: ...
 
def make_partial(name: Literal["schemas"]) -> Callback:
    def callback(self: 'Changes', *args, **kwargs) -> Statements:
        return statements_for_changes(getattr(self, name), *args, **kwargs)

class Changes():
    schemas = make_partial("schemas")  # should be a method with type Callback

However, the self arg in the methods cause mypy to vomit, so I'm stuck with repetitive method definitions.

SKalt avatar Jan 25 '20 22:01 SKalt