-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
Configure mypy and type annotate the code #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Round 1 and overall it looks very good.
I noticed not all files have from __future__ import annotations
at the top of the file. Please make sure this import is present.
src/Board.py
Outdated
def __init__(self, mateInOne: bool = False, castleBoard: bool = False, | ||
passant: bool = False, promotion: bool = False): | ||
self.pieces: List[Piece] = [] | ||
self.history: List[List] = [] # type: ignore[type-arg] # TODO: make 'history' contain only Move, not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually I'm blind, but i don't see when history would contain a None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See history
of a random game, just tested setting a breakpoint in main.py
:
history
[[<src.Move.Move object at 0x105a0d2a0>, None], [<src.Move.Move object at 0x105a0d0f0>, None], [<src.Move.Move object at 0x105a7ad70>, None], [<src.Move.Move object at 0x105a7acb0>, None], [<src.Move.Move object at 0x105a56500>, None], [<src.Move.Move object at 0x105a56440>, None], [<src.Move.Move object at 0x105c3a5c0>, None], [<src.Move.Move object at 0x105c3a680>, None], [<src.Move.Move object at 0x105a36620>, None], [<src.Move.Move object at 0x105a36590>, None], [<src.Move.Move object at 0x105c9c400>, None], [<src.Move.Move object at 0x105c3b130>, None], [<src.Move.Move object at 0x105c9c070>, None], [<src.Move.Move object at 0x105c1fd90>, <src.Bishop.Bishop object at 0x105a0c2b0>], [<src.Move.Move object at 0x105cf2a70>, None], [<src.Move.Move object at 0x105cf2e00>, None], [<src.Move.Move object at 0x105ce26e0>, None], [<src.Move.Move object at 0x105ce1fc0>, None], [<src.Move.Move object at 0x105d6af80>, None], [<src.Move.Move object at 0x105d6bd30>, <src.Pawn.Pawn object at 0x1059dff40>], [<src.Move.Move object at 0x105a65ab0>, None], [<src.Move.Move object at 0x105a64f70>, <src.Pawn.Pawn object at 0x105a0c100>], [<src.Move.Move object at 0x105cc0b80>, None], [<src.Move.Move object at 0x105cc0790>, None], [<src.Move.Move object at 0x105c163e0>, None], [<src.Move.Move object at 0x105c16d70>, None], [<src.Move.Move object at 0x105c56d70>, None], [<src.Move.Move object at 0x105c57790>, <src.Pawn.Pawn object at 0x1059dfbe0>], [<src.Move.Move object at 0x1068002e0>, None], [<src.Move.Move object at 0x106800700>, <src.Knight.Knight object at 0x105a0c250>], [<src.Move.Move object at 0x10688b7f0>, <src.Queen.Queen object at 0x106800f70>], [<src.Move.Move object at 0x10688bd00>, <src.Pawn.Pawn object at 0x1059dffa0>], [<src.Move.Move object at 0x106a3a4d0>, None], [<src.Move.Move object at 0x106a39fc0>, None]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
history
is declared to belist[list]
which is automatically translated tolist[list[Any]]
. So I assume mypy isn't happy for the following reasons:
- Line 166: mypy can't infer the return type because the element which is accessed (
history[-1][0]
where[0]
is the important part) is declared to beAny
- Line 171: same as in line 166. mypy can't infer what type
history[-1][0].piece
is becausehistory[-1][0]
is alreadyAny
.
In connection with this it would be an idea to change the type of history to be list[tuple[Piece, Optional[Piece]]]
. And all the places where this type is expected have to be changed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, I think you meant list[tuple[Move, Optional[Piece]]]
though. See if this makes sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant list[tuple[Move, Optional[Piece]]] though
Yeah, you're right with that.
Discussed fixes pushed, feel free to re-review when you have a sec. |
I felt so free and resolved the merging conflict created by merging #43. |
Fixed #35
This PR adds
mypy
to this project and the required types to the codebase.It also adds a bunch of TODOs as
mypy
uncovered a couple of issues within the code, like inconsistent types or missing returns. I didn't want to introduce any significant changes to the code as part of this PR so I decided to letmypy
ignore these for now, but with relevant TODOs so these can be addressed later.Running
mypy --show-error-codes --pretty --strict .
on this code:Success: no issues found in 16 source files
Running the same command on recent
master
:Found 370 errors in 14 files (checked 16 source files)