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.

Sign up for GitHub

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

Jump to bottom

Cleanup - fix flake8 errors and remove dead code #47

Merged

ClasherKasten merged 1 commit into marcusbuffett:master from aaaandrzej:cleanup

Oct 12, 2022

Contributor

aaaandrzej commented Oct 10, 2022

Fixing the first two points of [#33 ]:

Fix flake8 errors
removing dead code

aaaandrzej commented

View reviewed changes

.gitignore Show resolved Hide resolved

aaaandrzej commented

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

aaaandrzej commented

View reviewed changes

mypy.ini Show resolved Hide resolved

aaaandrzej commented

View reviewed changes

requirements.txt Show resolved Hide resolved

aaaandrzej commented

View reviewed changes

src/Board.py Outdated Show resolved Hide resolved

ClasherKasten reviewed

View reviewed changes

Collaborator

ClasherKasten left a comment •

edited

Loading

Round 1 and overall very good.
But this part with the hanging indents and multiline-statements inside of parantheses definitely have to be changed or if there are good arguments against a change, discussed.
And the weird

... C(
x, y
) ...

looks horrible.

.gitignore Outdated Show resolved Hide resolved

.flake8 Outdated Show resolved Hide resolved

src/Bishop.py Outdated

Comment on lines 29 to 32

+                          for move in self.movesInDirectionFromPos(
+                              currentPosition, direction, self.side
+                          ):

Collaborator

ClasherKasten Oct 10, 2022

When using a hanging indent the following should be considered; [...] further indentation should be used to clearly distinguish itself as a continuation line

As PEP8 says here. I think the standard is sometimes a bit vague. So this is open to the discussion how we want to handle that.

Contributor Author

aaaandrzej Oct 10, 2022

Maybe I'll just do another round of formatting and push it as a proposal which we can then discuss?

Collaborator

ClasherKasten Oct 10, 2022

Maybe I'll just do another round of formatting and push it as a proposal which we can then discuss?

Sounds like an idea. Approved 👍

src/Board.py Outdated Show resolved Hide resolved

src/King.py Outdated Show resolved Hide resolved

src/Move.py Outdated Show resolved Hide resolved

aaaandrzej requested a review from ClasherKasten

October 11, 2022 11:05

ClasherKasten reviewed

View reviewed changes

Collaborator

ClasherKasten left a comment

I marked some things from the last rounds (mostly the hanging indent thingy). I don't know why you only changed it in one or two files, but now I made a comment everywhere where it occurs.
Also i left some other comments. Please look at them and if something seems odd, we can discuss about it.

src/InputParser.py Outdated

Comment on lines 33 to 34

		self.board.getCoordinateNotationOfMove(move).lower()
		== notation.lower()

Collaborator

ClasherKasten Oct 11, 2022

I think we discussed this before. Please indent this correct.

Contributor Author

aaaandrzej Oct 12, 2022

It looked OK to me but sure, as you wish. This stuff is very subjective though and can easily be confused, plus 79 is a very strict limit, so if possible please suggest specific change here or even feel free to push it to this branch. Anyway, changing this one to:

            if self.board.getCoordinateNotationOfMove(
                    move).lower() == notation.lower():

src/InputParser.py Outdated

-                  def getLegalMovesWithNotation(self, side: bool, short: bool = True) -> list[Move]:
+                  def getLegalMovesWithNotation(
+                      self, side: bool, short: bool = True

Collaborator

ClasherKasten Oct 11, 2022

I think we discussed this before. Please indent this correct.

Contributor Author

aaaandrzej Oct 12, 2022

How would you want to format this to fit the line length limit?

I can do

    def getLegalMovesWithNotation(
            self, side: bool, short: bool = True) -> list[Move]:

as

    def getLegalMovesWithNotation(
        self, side: bool, short: bool = True) -> list[Move]:

makes flake8 complain

src/InputParser.py Outdated

Comment on lines 98 to 106

+                                  type(move.piece) is Pawn
+                                  and move.pieceToCapture
+                                  and re.sub(
+                                      '[1-8]',
+                                      '',
+                                      self.board.getCaptureNotation(move).replace('x', ''),
+                                  )
+                                  .lower()
+                                  .endswith(shortNotation)

Collaborator

ClasherKasten Oct 11, 2022

I think we discussed this before. Please indent this correct.

src/InputParser.py Outdated

Comment on lines 87 to 92

+                                  type(move.piece) is Pawn
+                                  and move.pieceToCapture
+                                  and self.board.getCaptureNotation(move)
+                                  .replace('x', '')
+                                  .lower()
+                                  .endswith(shortNotation)

Collaborator

ClasherKasten Oct 11, 2022

I think we discussed this before. Please indent this correct.

src/InputParser.py Outdated

Comment on lines 71 to 80

+                                  type(move.piece) is Pawn
+                                  and not move.pieceToCapture
+                                  and re.sub(
+                                      '[1-8]',
+                                      '',
+                                      self.board.getCoordinateNotationOfMove(move),
+                                  )
+                                  .replace('=', '')
+                                  .lower()
+                                  .endswith(shortNotation)

Collaborator

ClasherKasten Oct 11, 2022

I think we discussed this before. Please indent this correct.

Contributor Author

aaaandrzej Oct 12, 2022

Again, it looked very clear to me, so please suggest a specific change. I'm changing it to the below for now, although I don't like it myself:

                if type(move.piece) is Pawn and not move.pieceToCapture \
                        and re.sub(
                        '[1-8]', '', self.board.getCoordinateNotationOfMove(
                                move), ).replace('=', '').lower().endswith(
                        shortNotation):

src/Board.py Outdated

Comment on lines 167 to 168

		len(self.getAllMovesLegal(self.currentSide)) == 0
		and not self.isCheckmate()

Collaborator

ClasherKasten Oct 11, 2022

This only needs to be indented one times, not two times.

Contributor Author

aaaandrzej Oct 12, 2022

like this?

        return len(self.getAllMovesLegal(self.currentSide)) == 0 \
               and not self.isCheckmate()

src/Board.py Outdated

Comment on lines 235 to 239

+                                          fg_color
+                                          + bg_color
+                                          + DISPLAY_LOOKUP[piece.stringRep]
+                                          + ' '
+                                          + attr(0)

Collaborator

ClasherKasten Oct 11, 2022

This only need to be indented once, not two times.

Contributor Author

aaaandrzej Oct 12, 2022

Like that?

                    pieceRep = fg_color + bg_color + DISPLAY_LOOKUP[
                        piece.stringRep] + ' ' + attr(0)

src/Board.py Outdated

-                          notation += str(move.specialMovePiece.stringRep)  # type: ignore[attr-defined]
+                          notation += str(
+                              move.specialMovePiece.stringRep)  # type: ignore[attr-defined] # noqa: E501

Collaborator

ClasherKasten Oct 11, 2022

The paranthese should be moved onto a separate line here.

src/Board.py Outdated

-                          notation += str(move.specialMovePiece.stringRep)  # type: ignore[attr-defined, operator]
+                          notation += str(
+                              move.specialMovePiece.stringRep)  # type: ignore[attr-defined, operator]  # noqa: E501

Collaborator

ClasherKasten Oct 11, 2022

The paranthese should be moved onto a separate line here.

src/Board.py Outdated

                       notation += self.positionToHumanCoord(move.newPos)
                       if move.promotion:
-                          notation += "=" + str(move.specialMovePiece.stringRep)  # type: ignore[attr-defined]
+                          notation += '=' + str(
+                              move.specialMovePiece.stringRep)  # type: ignore[attr-defined]  # noqa: E501

Collaborator

ClasherKasten Oct 11, 2022

The paranthese should be moved onto a separate line here.

aaaandrzej requested a review from ClasherKasten

October 12, 2022 11:59

Contributor Author

aaaandrzej commented Oct 12, 2022

Round 3 pushed, although I'm still not sure i that's 100% what you expect. Feel free to suggest specific changes or push them directly if you wish, hopefully recent commit moves us in a good direction :)

ClasherKasten force-pushed the cleanup branch from 48633da to aa09c50 Compare

October 12, 2022 14:08

ClasherKasten approved these changes

View reviewed changes


          Add flake8, black and isort

ceb3cf6

Remove dead code

Make flake8 happy

Add .python-version to .gitignore

ClasherKasten force-pushed the cleanup branch from aa09c50 to ceb3cf6 Compare

October 12, 2022 14:10

ClasherKasten merged commit c2d9ae9 into marcusbuffett:master

ClasherKasten added the hacktoberfest-accepted label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted