NAS-129505 / 24.10 / Beginning of type hinting for middleware #13864

creatorcary · 2024-06-10T19:27:02Z

Type hinting is an internal documentation technique that does not affect the execution of code during runtime; type hinting is not actually enforced by the interpreter. Instead, it integrates with most IDE's and linters to make the development process cleaner by improving readability and allowing certain bugs to be noticed earlier.

bugclerk · 2024-06-10T19:27:17Z

Jira URL: https://ixsystems.atlassian.net/browse/NAS-129505

sonicaj · 2024-06-10T19:32:45Z

src/middlewared/middlewared/event.py

 from middlewared.schema import Any, clean_and_validate_arg, ValidationErrors
 from middlewared.settings import conf


 class Events:
-    def __init__(self, role_manager):
+    _events: dict[str, dict[str]]


Why move them to class variables ? We can specify their types in __init__ as well ?

You're right, fixed it

sonicaj · 2024-06-10T19:33:42Z

src/middlewared/middlewared/event.py

        self.role_manager = role_manager
        self._events = {}
        self.__events_private = set()

-    def register(self, name, description, private, returns, no_auth_required, no_authz_required, roles):
+    def register(self, name:str, description:str, private:bool, returns:bool, no_auth_required, no_authz_required, roles:Iterable[str]):


There should be a space after : i.e name: str (in PEP this is E231)

sonicaj · 2024-06-10T19:34:15Z

src/middlewared/middlewared/event.py

@@ -28,7 +33,7 @@ def register(self, name, description, private, returns, no_auth_required, no_aut
        if private:
            self.__events_private.add(name)

-    def get_event(self, name):
+    def get_event(self, name:str) -> dict[str]:


This can be null as well so we should mark it as such i.e typing.Optional

Also this would be typing.Dict[key_type, value_type]

Please see different modules in middleware which already have type hinting in place, that would be a nice point to see how we have been doing it so far

sonicaj · 2024-06-10T19:46:20Z

src/middlewared/middlewared/schema/attribute.py

@@ -80,7 +80,7 @@ def to_json_schema(self, parent=None):
        """
        raise NotImplementedError("Attribute must implement to_json_schema method")

-    def _to_json_schema_common(self, parent):
+    def _to_json_schema_common(self, parent) -> dict[str]:


typing.Dict[type_here, value_type_here]

As of Python 3.9 you can use just regular list and dict, but I'll change to the preferred List and Dict

I'm also learning the repo as I go, so Dict[str] evaluates to Dict[str, Any] since I wasn't sure

sonicaj · 2024-06-10T19:48:33Z

src/middlewared/middlewared/schema/attribute.py

@@ -12,7 +12,7 @@ class Attribute:

    def __init__(
        self, name='', title=None, description=None, required=False, null=False, empty=True, private=False,
-        validators=None, register=False, hidden=False, editable=True, example=None, **kwargs
+        validators:list[function]=None, register=False, hidden=False, editable=True, example=None, **kwargs


This would be syntax error i.e function is not defined anywhere so this won't execute and the correct way to do this is with typing.List

Also i think we should have a base validators class with __call__ method raising NotImplementedError and all validators inheriting from it and we can specify that validator here like this
typing.List[Validator]

I wasn't able to find a general Validator type, so I left it as List[Callable] for now.

Okay so to clarify, can we add something like this

class ValidatorBase: def __call__(self, *args, **kwargs): raise NotImplementedError() class Exact(ValidatorBase): def __init__(self, value): self.value = value def __call__(self, value): if value != self.value: raise ValueError(f"Should be {self.value!r}")

We can import the validator base here and then use it in type hints

Thanks for the thorough review, I've updated the PR with everything you pointed out.

…AS-129505

…into NAS-129505" This reverts commit 337937c, reversing changes made to ec1b54d.

…AS-129505

…iddleware into NAS-129505"" This reverts commit 31c6c29.

aiden3c · 2024-06-11T17:03:07Z

src/middlewared/middlewared/plugins/failover_/disabled_reasons.py

One of your merges into this branch looks like it's overwriting/undoing this commit. Doing a merge doesn't seem to fix it because (I think) you explicitly reverted the changes in your commit history on this branch.

You'll want to run git revert 31c6c2994cd343638a45a020f1da8b6c75a785c1 to revert the revert

bugclerk · 2024-06-12T13:10:12Z

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

sonicaj · 2024-06-12T13:03:48Z

src/middlewared/middlewared/event.py


-    def register(self, name, description, private, returns, no_auth_required, no_authz_required, roles):
+    def register(self, name: str, description: str, private: bool, returns, no_auth_required, no_authz_required, roles: typing.Iterable[str]):


no_auth_required/no_authz_required have been left out ?

If I came across anything that I wasn't sure about, I just skipped it. Figured it's better to be less specific than wrong, plus I plan on spending the whole week working on type hints/docstrings so I can go back to it.

sonicaj · 2024-06-12T13:06:37Z

src/middlewared/middlewared/main.py

@@ -1645,7 +1648,7 @@ def event_register(self, name, description, *, private=False, returns=None, no_a
        roles = roles or []
        self.events.register(name, description, private, returns, no_auth_required, no_authz_required, roles)

-    def send_event(self, name, event_type, **kwargs):
+    def send_event(self, name, event_type: str, **kwargs):


Missing type hint for name ?

I'll get back to this one

sonicaj · 2024-06-12T13:09:49Z

src/middlewared/middlewared/schema/attribute.py

@@ -1,8 +1,10 @@
 import copy
 import json
 import textwrap
+import typing


To be sure, we are not right now completing type hints here and would be done at a later stage ?

sonicaj · 2024-06-12T13:10:55Z

Oops, got a bit late :D

creatorcary added 2 commits June 10, 2024 12:45

Full type hinting in role.py

014fdd4

More type hints

c25574c

bugclerk changed the title ~~Beginning of type hinting for middleware~~ NAS-129505 / 24.10 / Beginning of type hinting for middleware Jun 10, 2024

creatorcary requested a review from a team June 10, 2024 19:27

creatorcary added the WIP label Jun 10, 2024

sonicaj reviewed Jun 10, 2024

View reviewed changes

creatorcary added 5 commits June 11, 2024 10:29

Fix PEP E231, ValidatorBase, typing module

ec1b54d

Merge branch 'master' of https://github.com/truenas/middleware into N…

337937c

…AS-129505

Revert "Merge branch 'master' of https://github.com/truenas/middleware …

31c6c29

…into NAS-129505" This reverts commit 337937c, reversing changes made to ec1b54d.

Fix dict type hints

25b6930

Add ExcInfoType

1ff7165

creatorcary requested a review from sonicaj June 11, 2024 16:28

Merge branch 'master' of https://github.com/truenas/middleware into N…

84f063c

…AS-129505

creatorcary removed the WIP label Jun 11, 2024

creatorcary added 2 commits June 11, 2024 15:52

Merge branch 'master' of https://github.com/truenas/middleware into N…

779cf5d

…AS-129505

Revert "Revert "Merge branch 'master' of https://github.com/truenas/m…

67ff523

…iddleware into NAS-129505"" This reverts commit 31c6c29.

aiden3c self-requested a review June 12, 2024 13:01

aiden3c approved these changes Jun 12, 2024

View reviewed changes

creatorcary merged commit 5556b0f into master Jun 12, 2024
3 checks passed

bugclerk added the no-time-tracked label Jun 12, 2024

truenas locked as resolved and limited conversation to collaborators Jun 12, 2024

sonicaj reviewed Jun 12, 2024

View reviewed changes

creatorcary deleted the NAS-129505 branch June 12, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NAS-129505 / 24.10 / Beginning of type hinting for middleware #13864

NAS-129505 / 24.10 / Beginning of type hinting for middleware #13864

creatorcary commented Jun 10, 2024

bugclerk commented Jun 10, 2024

sonicaj Jun 10, 2024

creatorcary Jun 10, 2024

sonicaj Jun 10, 2024 •

edited

Loading

sonicaj Jun 10, 2024

sonicaj Jun 10, 2024

sonicaj Jun 10, 2024

sonicaj Jun 10, 2024

creatorcary Jun 10, 2024

creatorcary Jun 10, 2024

sonicaj Jun 10, 2024

sonicaj Jun 10, 2024

creatorcary Jun 10, 2024

sonicaj Jun 10, 2024

creatorcary Jun 12, 2024

aiden3c Jun 11, 2024

aiden3c Jun 11, 2024

bugclerk commented Jun 12, 2024

sonicaj Jun 12, 2024

creatorcary Jun 12, 2024

sonicaj Jun 12, 2024

creatorcary Jun 12, 2024

sonicaj Jun 12, 2024

sonicaj commented Jun 12, 2024


		def register(self, name, description, private, returns, no_auth_required, no_authz_required, roles):
		def register(self, name: str, description: str, private: bool, returns, no_auth_required, no_authz_required, roles: typing.Iterable[str]):

NAS-129505 / 24.10 / Beginning of type hinting for middleware #13864

NAS-129505 / 24.10 / Beginning of type hinting for middleware #13864

Conversation

creatorcary commented Jun 10, 2024

bugclerk commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonicaj Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugclerk commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonicaj commented Jun 12, 2024

sonicaj Jun 10, 2024 •

edited

Loading