Add type annotations for gui/main and gui/widgets #358
Conversation
Type annotations were guessed, so they should be checked
I used `any` when I didn't know the type. I or someone should fix every `any`.
Dalek-Sec
requested review from
sssoleileraaa,
heartsucker and
redshiftzero
as code owners
There was a problem hiding this comment.
Heya. Thanks for the contribution. There's a few places it looks like we could scope the type checking a bit tighter so we don't have to use Any so frequently. I may be wrong about these (I didn't check locally).
Also before this can be merged, you're going to need to rebase this on the latest master and squash your commits into one. Let us know if you need help with this.
securedrop_client/gui/main.py
Outdated
| """ | ||
| Show the login form. | ||
| """ | ||
| self.login_dialog = LoginDialog(self) | ||
| self.login_dialog = LoginDialog(self) # type: Optional[LoginDialog] |
There was a problem hiding this comment.
Small refactor: We should define the type of this in the __init__ method so that on visual inspection of the class, all defined attributes are in one place. Additionally, setting it to None during __init__ prevents the case where accessing it would raise an AttributeError if we don't call setup, such as in testing. I know this slightly extends the scope of your work (minor refactor, not just adding annotations).
securedrop_client/gui/widgets.py
Outdated
| @@ -587,7 +587,7 @@ class MainView(QWidget): | |||
| } | |||
| ''' | |||
|
|
|||
| def __init__(self, parent): | |||
| def __init__(self, parent: Any) -> None: | |||
There was a problem hiding this comment.
I think this Any can be changed to QObject and pass type checking. Maybe?
securedrop_client/gui/widgets.py
Outdated
| @@ -837,7 +839,7 @@ def update(self): | |||
| if self.source.document_count == 0: | |||
| self.attached.hide() | |||
|
|
|||
| def delete_source(self, event): | |||
| def delete_source(self, event: Any) -> None: | |||
There was a problem hiding this comment.
This can be QEvent (I think). We might be able to scope it tighter if we check what delete_source is attached to.
securedrop_client/gui/widgets.py
Outdated
| @@ -908,12 +910,12 @@ def on_toggle_offline(self): | |||
| class DeleteSourceMessageBox: | |||
| """Use this to display operation details and confirm user choice.""" | |||
|
|
|||
| def __init__(self, parent, source, controller): | |||
| def __init__(self, parent: Any, source: Source, controller: Controller) -> None: | |||
securedrop_client/gui/widgets.py
Outdated
| self.parent = parent | ||
| super().__init__(self.parent) | ||
|
|
||
| def closeEvent(self, event): | ||
| def closeEvent(self, event: Any) -> None: |
securedrop_client/gui/widgets.py
Outdated
| update_signal, | ||
| message_succeeded_signal, | ||
| message_failed_signal, | ||
| update_signal: Any, |
There was a problem hiding this comment.
Any -> Function[...] (you'll have to double check the args here).
securedrop_client/gui/widgets.py
Outdated
| def __init__(self, source_db_object, submission_db_object, | ||
| controller, file_ready_signal, align="left"): | ||
| def __init__(self, source_db_object: Source, submission_db_object: Any, | ||
| controller: Controller, file_ready_signal: Any, align: str = "left") -> None: |
There was a problem hiding this comment.
submission_db_object: Any -> File
Signal: Any -> Function[...] (you'll have to double check the args here).
securedrop_client/gui/widgets.py
Outdated
| @@ -1332,7 +1334,7 @@ def _on_file_download(self, file_uuid: str) -> None: | |||
| self.clear() # delete existing icon and label | |||
| self.update() # draw modified widget | |||
|
|
|||
| def mouseReleaseEvent(self, e): | |||
| def mouseReleaseEvent(self, e: Any) -> None: | |||
securedrop_client/gui/widgets.py
Outdated
| @@ -1401,15 +1403,15 @@ def update_conversation(self, collection: list) -> None: | |||
| else: | |||
| self.add_file(self.source, conversation_item) | |||
|
|
|||
| def add_file(self, source_db_object, submission_db_object): | |||
| def add_file(self, source_db_object: Source, submission_db_object: Any) -> None: | |||
securedrop_client/gui/widgets.py
Outdated
| @@ -1560,7 +1562,7 @@ def _on_authentication_changed(self, authenticated: bool) -> None: | |||
| class DeleteSourceAction(QAction): | |||
| """Use this action to delete the source record.""" | |||
|
|
|||
| def __init__(self, source, parent, controller): | |||
| def __init__(self, source: Source, parent: Any, controller: Controller) -> None: | |||
|
Additionally, there were linting errors. You can test all this locally by running |
|
hey @Dalek-Sec! let us know if you want to continue working on this (addressing @heartsucker's comments), else we can have a maintainer take it over so we can get your changes merged 🚀 |
|
Thank you for the review, corrections, etc.! |
errors. Despite efforts to commit in a working state, the linter informs me that functions don't have an attribute `connect`.
|
@Dalek-Sec Just checking in, will you still be able to continue your work on this? |
If there's much left, sadly no. However, if there are only small problems remaining which I can fix on the weekend, I'm happy to finish what I started. In other words... I have no clue what I'm doing, but I want to be helpful, but I recalled resolving all the problems, so if there's more left, I probably really don't know what I'm doing, and it may be simpler to have someone else do it. |
|
Closing this for now since the original contributor was not able to finish work on it, feel free to re-open if you want to pick this up as-is in a future sprint. |
I left the args I had little clue about as
any. I also guessed several that I was less certain about even after grepping.