OpenOversight
OpenOversight copied to clipboard
lucyparsons
add type annotations
Once #736 is done, we should
- Add mypy to the dev dependencies
- Run mypy as part of the lint target to enable gradual typing
- Start adding PEP 484 style type annotations
This is a good way for folks to get familiar with the codebase
Can I claim this issue? I haven't done type annotations before but it seems like based on mypy docs the order of operations would be:
- Add mypy to the dev dependencies
- Run mypy as part of the lint target to enable gradual typing
- Correct any errors that come from original mypy run
- Ignore external modules that do not support type checking by adding them to the mypy.ini config
- Add type annotations to OpenOversight code that errors in the mypy run
- Begin to add PEP 484 style type annotations for all function definitions
My instinct also be that initially we should just worry about files within OpenOversight/app so that would be the directory I would set for the lint target but if anyone has experience with using mypy on tests let me know!
Hi @egfrank. Yes, you can claim this issue!
In regards to your plan, I think adding mypy to dev requirements and linting can be done independently from adding type annotations. Also I am not sure which errors we can expect mypy to show before having added type-annotations, so fixing errors might be at the end of this, also depending on how many issues come out of it.
Finally, yes, focusing on files in OpenOversight/app makes a lot of sense to me
Hi, thanks for the response. I totally agree that mypy to dev requirements and linting can be done independently from adding type annotations and in fact I have commits for the dev requirements and linting if you want me to push those right away.
That being said, if you run mypy on the develop branch here are the errors you get (note: this is still using a mypy.ini config file that mutes errors on certain imports if it cannot figure out their type. If you ran mypy without this config I think you'd get ~40 more errors saying they cannot infer types from xxx package).
OpenOversight/app/models.py:26: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:44: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:64: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:77: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:90: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:155: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:170: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:189: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:201: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:241: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:285: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:323: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:337: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:354: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:379: error: Name 'db.Model' is not defined
OpenOversight/app/model_imports.py:36: error: Module has no attribute "parser"
OpenOversight/app/model_imports.py:42: error: Module has no attribute "parser"
OpenOversight/app/main/views.py:1118: error: Incompatible types in assignment (expression has type "Type[Incident]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1122: error: Incompatible types in assignment (expression has type "Type[IncidentForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1123: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1250: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1279: error: Incompatible types in assignment (expression has type "Type[Note]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1281: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1282: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1289: error: Incompatible types in assignment (expression has type "Type[Description]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1291: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1292: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/commands.py:59: error: Cannot find implementation or library stub for module named 'app.models'
OpenOversight/app/commands.py:59: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
OpenOversight/app/commands.py:91: error: Need type annotation for 'updated_officers' (hint: "updated_officers: Dict[<type>, <type>] = ...")
OpenOversight/app/commands.py:92: error: Need type annotation for 'created_officers' (hint: "created_officers: Dict[<type>, <type>] = ...")
Found 30 errors in 4 files (checked 20 source files)
So what I'd suggest is in the first pull request, also correcting these errors so that if someone runs make lint on develop then it is passing with exit code 0. Most of these errors can just be corrected by adding either a type annotation to a variable (like db.Model) or even just type: ignore if it seems like the error is not useful.
Additionally though, I was reading this blog post http://calpaterson.com/mypy-hints.html and they recommend adding two config options to make mypy more strict. They say
check_untyped_defs makes mypy attempt to check the interiors of functions that don't have type annotations. Otherwise, in projects with low levels of type annotation, mypy is doing very little.
no_implicit_optional will raise type errors when you assume arguments are not None when in fact they are nullable.
Running on develop with those two options true looks like
OpenOversight/app/config.py:73: error: "Dict[str, Type[BaseConfig]]" has no attribute "init_app"
OpenOversight/app/formfields.py:38: error: Incompatible types in assignment (expression has type "None", variable has type "time")
OpenOversight/app/models.py:26: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:44: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:64: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:77: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:90: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:155: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:170: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:189: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:201: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:241: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:285: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:323: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:337: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:354: error: Name 'db.Model' is not defined
OpenOversight/app/models.py:379: error: Name 'db.Model' is not defined
OpenOversight/app/utils.py:481: error: Incompatible types in assignment (expression has type "BinaryIO", variable has type "BytesIO")
OpenOversight/app/utils.py:481: error: Value of type variable "AnyStr" of "abspath" cannot be "Optional[str]"
OpenOversight/app/model_imports.py:36: error: Module has no attribute "parser"
OpenOversight/app/model_imports.py:42: error: Module has no attribute "parser"
OpenOversight/app/main/model_view.py:23: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
OpenOversight/app/main/model_view.py:29: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:30: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:32: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:36: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:54: error: "str" not callable
OpenOversight/app/main/model_view.py:67: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:97: error: "None" has no attribute "query"
OpenOversight/app/main/model_view.py:111: error: "str" not callable
OpenOversight/app/main/model_view.py:115: error: "str" not callable
OpenOversight/app/main/model_view.py:136: error: "None" not callable
OpenOversight/app/main/views.py:521: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
OpenOversight/app/main/views.py:527: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
OpenOversight/app/main/views.py:885: error: Need type annotation for 'assign_dict' (hint: "assign_dict: Dict[<type>, <type>] = ...")
OpenOversight/app/main/views.py:1118: error: Incompatible types in assignment (expression has type "Type[Incident]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1122: error: Incompatible types in assignment (expression has type "Type[IncidentForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1123: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1128: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
OpenOversight/app/main/views.py:1134: error: Cannot determine type of 'model'
OpenOversight/app/main/views.py:1140: error: Cannot determine type of 'form'
OpenOversight/app/main/views.py:1250: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1253: error: Cannot determine type of 'form'
OpenOversight/app/main/views.py:1279: error: Incompatible types in assignment (expression has type "Type[Note]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1281: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1282: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1289: error: Incompatible types in assignment (expression has type "Type[Description]", base class "ModelView" defined the type as "None")
OpenOversight/app/main/views.py:1291: error: Incompatible types in assignment (expression has type "Type[TextForm]", base class "ModelView" defined the type as "str")
OpenOversight/app/main/views.py:1292: error: Incompatible types in assignment (expression has type "Callable[[Any, Any], Any]", base class "ModelView" defined the type as "str")
OpenOversight/app/auth/views.py:249: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, _SupportsIndex]"
OpenOversight/app/commands.py:59: error: Cannot find implementation or library stub for module named 'app.models'
OpenOversight/app/commands.py:59: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
OpenOversight/app/commands.py:91: error: Need type annotation for 'updated_officers' (hint: "updated_officers: Dict[<type>, <type>] = ...")
OpenOversight/app/commands.py:92: error: Need type annotation for 'created_officers' (hint: "created_officers: Dict[<type>, <type>] = ...")
OpenOversight/app/commands.py:386: error: Need type annotation for 'departments' (hint: "departments: Dict[<type>, <type>] = ...")
OpenOversight/app/commands.py:396: error: Unsupported right operand type for in ("Optional[Sequence[str]]")
OpenOversight/app/commands.py:399: error: Unsupported right operand type for in ("Optional[Sequence[str]]")
OpenOversight/app/commands.py:400: error: Unsupported right operand type for in ("Optional[Sequence[str]]")
OpenOversight/app/commands.py:415: error: Unsupported right operand type for in ("Optional[Sequence[str]]")
OpenOversight/app/commands.py:420: error: Unsupported right operand type for in ("Optional[Sequence[str]]")
Found 59 errors in 9 files (checked 20 source files)
I'm definitely willing to check out all these errors if you think those config options make sense, and have already started looking into some of them.
Please let me know how many changes you want in the 1st pull request, and if you want me to split off this issue into further issues - I'd be more than happy to type up issues explaining what I think the next steps are.
Nice research, thanks for looking into this very thoroughly!
So what I'd suggest is in the first pull request, also correcting these errors so that if someone runs make lint on develop then it is passing with exit code 0. Most of these errors can just be corrected by adding either a type annotation to a variable (like db.Model) or even just type: ignore if it seems like the error is not useful.
This sounds like a great first goal to me
Additionally though, I was reading this blog post http://calpaterson.com/mypy-hints.html and they recommend adding two config options to make mypy more strict.
Yeah, I am definitely on board with no_implicit_optional, forgetting that something might be None is certainly of the most common errors. I am also fine with check_untyped_defs although it's not as clear to me what it does exactly
Please let me know how many changes you want in the 1st pull request, and if you want me to split off this issue into further issues - I'd be more than happy to type up issues explaining what I think the next steps are.
Good question, but I think whatever you feel best with is fine. Both in terms of having smaller or bigger PRs and in terms of adding additional issues. I do think that having a PR for the dev-setup of mypy right away is a nice idea, so we can start using it for other incoming PRs, but also fine if you prefer to fix up the errors first
Okay great I'll make a PR with just the dev changes right now. Do I need to get any permissions to make a PR? I'm currently getting a 403 when I try to push a branch. Sorry if I'm missing some instructions from the Contrib file.
Right, I am sorry, I just realized that our contributions guide is not giving the right instructions. You will have to fork this repository, push the branch to your fork and then you can create a PR from your repository to this one. Here is more information on that: https://guides.github.com/activities/forking/
Sorry this too so long to get back to you! I liked the idea of splitting this up into two PRs, one just for the Dev Dependencies and one for resolving all those issues. I was able to get to both today. Should be obvious but I'd recommend looking at
https://github.com/lucyparsons/OpenOversight/pull/830
before looking at
https://github.com/lucyparsons/OpenOversight/pull/831
Although 831 uses the single commit from 830 so technically you could just merge that one
Thanks! I think it's fine to just merge #831. Great work on containing all these errors with surprisingly few changes.