chore: mypy: Disallow untyped definitions

JohnVillalovos · JohnVillalovos · commit 6aef2dadf715 · 2021-04-27T21:04:58.000-07:00
Be more strict and don't allow untyped definitions on the files we
check.

Also this adds type-hints for two of the decorators so that now
functions/methods decorated by them will have their types be revealed
correctly.
diff --git a/.mypy.ini b/.mypy.ini
@@ -4,3 +4,6 @@ files = gitlab/*.py
 # disallow_incomplete_defs: This flag reports an error whenever it encounters a
 # partly annotated function definition.
 disallow_incomplete_defs = True
+# disallow_untyped_defs: This flag reports an error whenever it encounters a
+# function without type annotations or with incomplete type annotations.
+disallow_untyped_defs = True
diff --git a/gitlab/__main__.py b/gitlab/__main__.py
@@ -1,4 +1,5 @@
 import gitlab.cli
 
 
-__name__ == "__main__" and gitlab.cli.main()
+if __name__ == "__main__":
+    gitlab.cli.main()
diff --git a/gitlab/base.py b/gitlab/base.py
@@ -17,7 +17,7 @@
 
 import importlib
 from types import ModuleType
-from typing import Any, Dict, NamedTuple, Optional, Tuple, Type
+from typing import Any, Dict, Iterable, NamedTuple, Optional, Tuple, Type
 
 from .client import Gitlab, GitlabList
 from gitlab import types as g_types
@@ -133,8 +133,8 @@ def __ne__(self, other: object) -> bool:
             return self.get_id() != other.get_id()
         return super(RESTObject, self) != other
 
-    def __dir__(self):
-        return super(RESTObject, self).__dir__() | self.attributes.keys()
+    def __dir__(self) -> Iterable[str]:
+        return set(self.attributes).union(super(RESTObject, self).__dir__())
 
     def __hash__(self) -> int:
         if not self.get_id():
@@ -155,7 +155,7 @@ def _update_attrs(self, new_attrs: Dict[str, Any]) -> None:
         self.__dict__["_updated_attrs"] = {}
         self.__dict__["_attrs"] = new_attrs
 
-    def get_id(self):
+    def get_id(self) -> Any:
         """Returns the id of the resource."""
         if self._id_attr is None or not hasattr(self, self._id_attr):
             return None
@@ -207,10 +207,10 @@ def __iter__(self) -> "RESTObjectList":
     def __len__(self) -> int:
         return len(self._list)
 
-    def __next__(self):
+    def __next__(self) -> RESTObject:
         return self.next()
 
-    def next(self):
+    def next(self) -> RESTObject:
         data = self._list.next()
         return self._obj_cls(self.manager, data)
 
diff --git a/gitlab/cli.py b/gitlab/cli.py
@@ -21,7 +21,7 @@
 import functools
 import re
 import sys
-from typing import Any, Callable, Dict, Optional, Tuple, Union
+from typing import Any, Callable, cast, Dict, Optional, Tuple, TypeVar, Union
 
 import gitlab.config  # noqa: F401
 
@@ -35,14 +35,21 @@
 custom_actions: Dict[str, Dict[str, Tuple[Tuple[str, ...], Tuple[str, ...], bool]]] = {}
 
 
+# For an explanation of how these type-hints work see:
+# https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
+#
+# The goal here is that functions which get decorated will retain their types.
+__F = TypeVar("__F", bound=Callable[..., Any])
+
+
 def register_custom_action(
     cls_names: Union[str, Tuple[str, ...]],
     mandatory: Tuple[str, ...] = tuple(),
     optional: Tuple[str, ...] = tuple(),
-) -> Callable:
-    def wrap(f: Callable) -> Callable:
+) -> Callable[[__F], __F]:
+    def wrap(f: __F) -> __F:
         @functools.wraps(f)
-        def wrapped_f(*args, **kwargs):
+        def wrapped_f(*args: Any, **kwargs: Any) -> Any:
             return f(*args, **kwargs)
 
         # in_obj defines whether the method belongs to the obj or the manager
@@ -63,7 +70,7 @@ def wrapped_f(*args, **kwargs):
             action = f.__name__.replace("_", "-")
             custom_actions[final_name][action] = (mandatory, optional, in_obj)
 
-        return wrapped_f
+        return cast(__F, wrapped_f)
 
     return wrap
 
@@ -135,12 +142,16 @@ def _get_base_parser(add_help: bool = True) -> argparse.ArgumentParser:
     return parser
 
 
-def _get_parser(cli_module):
+def _get_parser() -> argparse.ArgumentParser:
+    # NOTE: We must delay import of gitlab.v4.cli until now or
+    # otherwise it will cause circular import errors
+    import gitlab.v4.cli
+
     parser = _get_base_parser()
-    return cli_module.extend_parser(parser)
+    return gitlab.v4.cli.extend_parser(parser)
 
 
-def _parse_value(v):
+def _parse_value(v: Any) -> Any:
     if isinstance(v, str) and v.startswith("@"):
         # If the user-provided value starts with @, we try to read the file
         # path provided after @ as the real value. Exit on any error.
@@ -162,18 +173,10 @@ def docs() -> argparse.ArgumentParser:
     if "sphinx" not in sys.modules:
         sys.exit("Docs parser is only intended for build_sphinx")
 
-    # NOTE: We must delay import of gitlab.v4.cli until now or
-    # otherwise it will cause circular import errors
-    import gitlab.v4.cli
-
-    return _get_parser(gitlab.v4.cli)
-
+    return _get_parser()
 
-def main():
-    # NOTE: We must delay import of gitlab.v4.cli until now or
-    # otherwise it will cause circular import errors
-    import gitlab.v4.cli
 
+def main() -> None:
     if "--version" in sys.argv:
         print(gitlab.__version__)
         sys.exit(0)
@@ -183,7 +186,7 @@ def main():
     # This first parsing step is used to find the gitlab config to use, and
     # load the propermodule (v3 or v4) accordingly. At that point we don't have
     # any subparser setup
-    (options, args) = parser.parse_known_args(sys.argv)
+    (options, _) = parser.parse_known_args(sys.argv)
     try:
         config = gitlab.config.GitlabConfigParser(options.gitlab, options.config_file)
     except gitlab.config.ConfigError as e:
@@ -196,14 +199,14 @@ def main():
         raise ModuleNotFoundError(name="gitlab.v%s.cli" % config.api_version)
 
     # Now we build the entire set of subcommands and do the complete parsing
-    parser = _get_parser(gitlab.v4.cli)
+    parser = _get_parser()
     try:
         import argcomplete  # type: ignore
 
         argcomplete.autocomplete(parser)
     except Exception:
         pass
-    args = parser.parse_args(sys.argv[1:])
+    args = parser.parse_args()
 
     config_files = args.config_file
     gitlab_id = args.gitlab
@@ -216,7 +219,7 @@ def main():
     action = args.whaction
     what = args.what
 
-    args = args.__dict__
+    args_dict = vars(args)
     # Remove CLI behavior-related args
     for item in (
         "gitlab",
@@ -228,8 +231,8 @@ def main():
         "version",
         "output",
     ):
-        args.pop(item)
-    args = {k: _parse_value(v) for k, v in args.items() if v is not None}
+        args_dict.pop(item)
+    args_dict = {k: _parse_value(v) for k, v in args_dict.items() if v is not None}
 
     try:
         gl = gitlab.Gitlab.from_config(gitlab_id, config_files)
@@ -241,6 +244,4 @@ def main():
     if debug:
         gl.enable_debug()
 
-    gitlab.v4.cli.run(gl, what, action, args, verbose, output, fields)
-
-    sys.exit(0)
+    gitlab.v4.cli.run(gl, what, action, args_dict, verbose, output, fields)
diff --git a/gitlab/config.py b/gitlab/config.py
@@ -206,7 +206,7 @@ def __init__(
         except Exception:
             pass
 
-    def _get_values_from_helper(self):
+    def _get_values_from_helper(self) -> None:
         """Update attributes that may get values from an external helper program"""
         for attr in HELPER_ATTRIBUTES:
             value = getattr(self, attr)
diff --git a/gitlab/exceptions.py b/gitlab/exceptions.py
@@ -16,10 +16,16 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import functools
+from typing import Any, Callable, cast, Optional, Type, TypeVar, TYPE_CHECKING, Union
 
 
 class GitlabError(Exception):
-    def __init__(self, error_message="", response_code=None, response_body=None):
+    def __init__(
+        self,
+        error_message: Union[str, bytes] = "",
+        response_code: Optional[int] = None,
+        response_body: Optional[bytes] = None,
+    ) -> None:
 
         Exception.__init__(self, error_message)
         # Http status code
@@ -30,11 +36,15 @@ def __init__(self, error_message="", response_code=None, response_body=None):
         try:
             # if we receive str/bytes we try to convert to unicode/str to have
             # consistent message types (see #616)
+            if TYPE_CHECKING:
+                assert isinstance(error_message, bytes)
             self.error_message = error_message.decode()
         except Exception:
+            if TYPE_CHECKING:
+                assert isinstance(error_message, str)
             self.error_message = error_message
 
-    def __str__(self):
+    def __str__(self) -> str:
         if self.response_code is not None:
             return "{0}: {1}".format(self.response_code, self.error_message)
         else:
@@ -269,7 +279,14 @@ class GitlabUnfollowError(GitlabOperationError):
     pass
 
 
-def on_http_error(error):
+# For an explanation of how these type-hints work see:
+# https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
+#
+# The goal here is that functions which get decorated will retain their types.
+__F = TypeVar("__F", bound=Callable[..., Any])
+
+
+def on_http_error(error: Type[Exception]) -> Callable[[__F], __F]:
     """Manage GitlabHttpError exceptions.
 
     This decorator function can be used to catch GitlabHttpError exceptions
@@ -280,14 +297,14 @@ def on_http_error(error):
             GitlabError
     """
 
-    def wrap(f):
+    def wrap(f: __F) -> __F:
         @functools.wraps(f)
-        def wrapped_f(*args, **kwargs):
+        def wrapped_f(*args: Any, **kwargs: Any) -> Any:
             try:
                 return f(*args, **kwargs)
             except GitlabHttpError as e:
                 raise error(e.error_message, e.response_code, e.response_body) from e
 
-        return wrapped_f
+        return cast(__F, wrapped_f)
 
     return wrap
diff --git a/gitlab/tests/test_cli.py b/gitlab/tests/test_cli.py
@@ -26,7 +26,6 @@
 import pytest
 
 from gitlab import cli
-import gitlab.v4.cli
 
 
 def test_what_to_cls():
@@ -94,14 +93,14 @@ def test_base_parser():
 
 
 def test_v4_parse_args():
-    parser = cli._get_parser(gitlab.v4.cli)
+    parser = cli._get_parser()
     args = parser.parse_args(["project", "list"])
     assert args.what == "project"
     assert args.whaction == "list"
 
 
 def test_v4_parser():
-    parser = cli._get_parser(gitlab.v4.cli)
+    parser = cli._get_parser()
     subparsers = next(
         action
         for action in parser._actions
diff --git a/gitlab/types.py b/gitlab/types.py
@@ -15,46 +15,50 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+from typing import Any, Optional, TYPE_CHECKING
+
 
 class GitlabAttribute(object):
-    def __init__(self, value=None):
+    def __init__(self, value: Any = None) -> None:
         self._value = value
 
-    def get(self):
+    def get(self) -> Any:
         return self._value
 
-    def set_from_cli(self, cli_value):
+    def set_from_cli(self, cli_value: Any) -> None:
         self._value = cli_value
 
-    def get_for_api(self):
+    def get_for_api(self) -> Any:
         return self._value
 
 
 class ListAttribute(GitlabAttribute):
-    def set_from_cli(self, cli_value):
+    def set_from_cli(self, cli_value: str) -> None:
         if not cli_value.strip():
             self._value = []
         else:
             self._value = [item.strip() for item in cli_value.split(",")]
 
-    def get_for_api(self):
+    def get_for_api(self) -> str:
         # Do not comma-split single value passed as string
         if isinstance(self._value, str):
             return self._value
 
+        if TYPE_CHECKING:
+            assert isinstance(self._value, list)
         return ",".join([str(x) for x in self._value])
 
 
 class LowercaseStringAttribute(GitlabAttribute):
-    def get_for_api(self):
+    def get_for_api(self) -> str:
         return str(self._value).lower()
 
 
 class FileAttribute(GitlabAttribute):
-    def get_file_name(self, attr_name=None):
+    def get_file_name(self, attr_name: Optional[str] = None) -> Optional[str]:
         return attr_name
 
 
 class ImageAttribute(FileAttribute):
-    def get_file_name(self, attr_name=None):
+    def get_file_name(self, attr_name: Optional[str] = None) -> str:
         return "%s.png" % attr_name if attr_name else "image.png"
