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

sambacc: complete type annotations #47

Merged

Conversation

phlogistonjohn
Copy link
Collaborator

The sambacc dir is basically a library and we want to ensure high quality and good documentation so configure mypy to require
type annotations on functions defined in sambacc. Update code in sambacc to add type annotations to any functions lacking them.

The sambacc.commands on the other hand are mean to be only run as part of our CLI tools so we'll be a bit less strict there.

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

Two minor comments. Overall, LGTM.

Comment on lines 37 to 38
self.ref = ref
self.items = items
self.items = items or []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider having (align with other member-initialization in this PR):

       self.ref: str = ref
       self.items: list[HostInfo] = items or []

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.

@@ -85,7 +85,7 @@ def _get_events(self):
and ((event.mask & _inotify.flags.CLOSE_WRITE) != 0)
]

def _wait(self):
def _wait(self) -> typing.Iterator[None]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why no just have a single wait function? What is the advantage of using next(typing.Iterator[None]) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the time I must have felt implementing the function in terms of an iterator was easier or clearer. This PR only adds the proper type annotation to the function, it doesn't change the runtime behavior. Let's leave that for another time.

@phlogistonjohn
Copy link
Collaborator Author

Wow. Somehow I managed to forget about this PR. I will need to revisit this soon!

This includes a small change to the signature of the watch function
to only accept a print_func callable and never none. This required
a small update to the test code. The real code always passed a function
and the ability to pass none never really could have worked right
anyway.


Signed-off-by: John Mulligan <[email protected]>
The sambacc dir is basically a library and we want to ensure high
quality and good documentation so configure mypy to require
type annotations on functions defined in sambacc.
The sambacc.commands on the other hand are mean to be only
run as part of our CLI tools so we'll be a bit less strict there.


Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn
Copy link
Collaborator Author

I finally updated this PR. wow.
@synarete @spuiuk Please revisit. I'd appreciate reviews within a week's time, so I'm putting on a virtual deadline of 2022-11-15. thanks!

Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +162 to 165
def smbd_foreground() -> SambaCommand:
return smbd[
"--foreground", _daemon_stdout_opt("smbd"), "--no-process-group"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, now that we have explicit return type, those lines look a bit enigmatic. I would consider (in a separate PR) to have more explicit code (also in other places below):

def smbd_foreground() -> SambaCommand:
    return SambaCommand("/usr/sbin/smbd",
        [ "--foreground", _daemon_stdout_opt("smbd"), "--no-process-group" ])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll think about it. On the other hand, the square-braces after a SambaCommand object gives you a new SambaCommand object with additional arguments was there from the beginning as a convenient, don't need to repeat yourself, construct.

@phlogistonjohn phlogistonjohn merged commit 756c8c4 into samba-in-kubernetes:master Nov 16, 2022
@phlogistonjohn phlogistonjohn deleted the jjm-even-more-typing branch November 16, 2022 16:22
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.

2 participants