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

Add .mypy.ini, pylintrc and satisfy pylint, mypy checks #11

Merged
merged 6 commits into from
Feb 5, 2022
Merged

Conversation

Yo1k
Copy link
Owner

@Yo1k Yo1k commented Feb 1, 2022

No description provided.

@Yo1k Yo1k requested a review from stIncMale February 1, 2022 21:17
@Yo1k Yo1k self-assigned this Feb 1, 2022
from __future__ import annotations
from typing import TypeVar, Type

T = TypeVar("T") # pylint: disable=C0103
Copy link
Owner Author

Choose a reason for hiding this comment

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

Pylint 2.12 (or earlier versions) does not work properly with TypeVar in the (it is fixed in the 2.13).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please create a GitHub issue to update to Pylint 2.13 when it comes out, and to remove the suppression here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

return isinstance(other, self.__class__) and self.__dict__ == other.__dict__
cls.__eq__ = __eq__
cls.__eq__ = __eq__ # type: ignore
Copy link
Owner Author

Choose a reason for hiding this comment

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

Mypy do not allow assigning a function to a method (see here).

@@ -112,10 +112,10 @@ def test_win_condition(self):
Cell(0, 2), False)]
for cells, cell, expect in args_and_expect_list:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Mypy does not inference the right type for cells and the syntax does not allow one to annotate the types of variables when tuple unpacking is used.

Copy link
Collaborator

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

Good job 👍 Just a few little things.

from __future__ import annotations
from typing import TypeVar, Type

T = TypeVar("T") # pylint: disable=C0103
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please create a GitHub issue to update to Pylint 2.13 when it comes out, and to remove the suppression here?

.pylintrc Show resolved Hide resolved
yo1k/tic_tac_toe/test/core/test_game.py Outdated Show resolved Hide resolved
yo1k/tic_tac_toe/test/core/test_game.py Show resolved Hide resolved
yo1k/tic_tac_toe/core/game.py Outdated Show resolved Hide resolved
@@ -256,23 +259,23 @@ def __last_step(step: int) -> bool:
return step == Board.size() ** 2 - 1

@staticmethod
def __surrender(state: State):
def __surrender(state: State) -> None:
assert state.phase is Phase.INROUND
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add another assertion:

# for more players this method would have been implemented quite differently
assert State.player_count() is 2

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I left a comment in the example for a reason: I think we need to clarify this new assertion.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

@stIncMale stIncMale linked an issue Feb 2, 2022 that may be closed by this pull request
@Yo1k Yo1k requested a review from stIncMale February 3, 2022 20:15
@@ -256,23 +259,23 @@ def __last_step(step: int) -> bool:
return step == Board.size() ** 2 - 1

@staticmethod
def __surrender(state: State):
def __surrender(state: State) -> None:
assert state.phase is Phase.INROUND
Copy link
Collaborator

Choose a reason for hiding this comment

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

I left a comment in the example for a reason: I think we need to clarify this new assertion.

@Yo1k Yo1k requested a review from stIncMale February 4, 2022 21:02
@Yo1k Yo1k merged commit f90c721 into master Feb 5, 2022
@Yo1k Yo1k deleted the linter_usage branch February 5, 2022 14:40
@stIncMale stIncMale added this to the singleplayer milestone Feb 13, 2022
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