Typehints: Adding type hints to the parameters and fixtures of test_messages.py #1373
Conversation
|
@zulipbot add "PR needs review" |
|
@abdulmoiz37 I'm taking a longer look, but just a note that the linting is likely simply that we use tools to autoformat the code (black, isort), but we don't enforce that individuals run them on each commit. See https://github.com/zulip/zulip-terminal#auto-formatting-code. |
There was a problem hiding this comment.
@abdulmoiz37 Thanks for giving this a go! I'd recommend reviewing other test files a little when doing this, side by side, for where the types go.
Some of the other types will get checked more automatically when they are in the right places, as I noted inline.
tests/ui_tools/test_messages.py
Outdated
| @@ -25,12 +28,12 @@ | |||
|
|
|||
| class TestMessageBox: | |||
| @pytest.fixture(autouse=True) | |||
| def mock_external_classes(self, mocker, initial_index): | |||
| def mock_external_classes(self, mocker: MockerFixture, initial_index: int) -> Any: | |||
There was a problem hiding this comment.
initial_index is a common fixture. See conftest.py for a lot of these.
I suspect this should return None.
There was a problem hiding this comment.
Okay,No problem I will check edit this and create a new commit
tests/ui_tools/test_messages.py
Outdated
| self.model = mocker.MagicMock() | ||
| self.model.index = initial_index | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "message_type, set_fields", | ||
| "message_type: str, set_fields:List[Tuple[str, Any]]", |
There was a problem hiding this comment.
Check the other files which have been annotated - these belong in the def test_init(...) part of the function.
There was a problem hiding this comment.
Note the lines you changed here should not be updated in the PR - pytest fails due to this. The changes should be in the other part, which I see you've changed in some locations.
There was a problem hiding this comment.
@neiljp Do you want me remove this latest commit and come up with a new single commit for all the changes in this PR?
There was a problem hiding this comment.
You can add commits as you like, and I can give limited feedback based on the overall changes at an earlier stage. However, as per the above link, the easier it is to read the better. One commit per file with type annotations added should be fine.
neiljp
added
PR awaiting update
|
Also note that we don't use merge commits so these will need removing, though I'm aware you wanted early feedback at this time 👍 |
zulipbot
added
size: XL
|
@neiljp Sir ,If the above commit Satisfies the Type hints test_messages.py and annotations should i also add this in |
|
@abdulmoiz37 Other PRs would be fine, though I'd wait for this to be complete first, to ensure you understand the requirements. |
I Have Opened this PR for the #1337 but dont know why the linting tests are failing @neiljp. Plz do have a review of this