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.

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

Fix some code style issues #837

Closed
wants to merge 6 commits into from
Closed

Fix some code style issues #837

wants to merge 6 commits into from

Conversation

Gatsik
Copy link
Contributor

@Gatsik Gatsik commented Sep 21, 2021

There are left some # noqa:s, and currently I have no idea how to properly fix them

@codecov
Copy link

codecov bot commented Sep 21, 2021 •
edited
Loading

Codecov Report

Merging #837 (57c81da) into develop (5cb90fa) will decrease coverage by 0.14%.
The diff coverage is 75.60%.

Impacted Files Coverage Δ
server/api/api_accessor.py 91.83% <ø> (ø)
server/exceptions.py 100.00% <ø> (ø)
server/game_service.py 99.01% <ø> (ø)
server/games/coop.py 100.00% <ø> (ø)
server/games/custom_game.py 100.00% <ø> (ø)
server/ice_servers/nts.py 100.00% <ø> (ø)
server/matchmaker/algorithm/stable_marriage.py 99.03% <ø> (ø)
server/matchmaker/algorithm/team_matchmaker.py 96.93% <ø> (ø)
server/message_queue_service.py 97.84% <ø> (ø)
server/rating.py 93.58% <ø> (ø)
... and 17 more

@Gatsik
Copy link
Contributor Author

Gatsik commented Sep 21, 2021 •
edited
Loading

Irony: by fixing flake8's style issues (F821 undefined name) I've added Codacy Static Code Analysis issues. FeelsBadMan

Gatsik
Gatsik commented Sep 21, 2021
View reviewed changes
server/games/custom_game.py
@@ -1,4 +1,3 @@
import asyncio
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and why this was not marked as unused import before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I am seeing an unused import warning for this using flake8. It might have been in there for a long time though, like since before we added codacy?

Gatsik
Gatsik commented Sep 21, 2021
View reviewed changes
server/games/coop.py
@@ -1,5 +1,3 @@
import asyncio
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this

Askaholic
Askaholic reviewed Sep 23, 2021
View reviewed changes
Copy link
Collaborator

@Askaholic Askaholic left a comment

Choose a reason for hiding this comment

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

Awesome work, these are some great fixes! Looking forward to merging this.

The only ones I'm not a fan of are the conditional imports. I don't think they're needed and they're going to mess with isort as well as code coverage.

server/broadcast_service.py
@@ -9,6 +13,9 @@
from .player_service import PlayerService
from .timing import LazyIntervalTimer

if TYPE_CHECKING:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the if check is needed here. At least flake8 doesn't generate unused import warnings here. Hopefully its the same on codacy

Copy link
Contributor Author

@Gatsik Gatsik Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The check is needed because of import cycle. If you remove import, then flake8 will report the undefined name error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, then how is it possible for the type checker to import it?

Copy link
Contributor Author

@Gatsik Gatsik Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Type checkers assume this constant (TYPE_CHECKING) to be True. (mypy docs)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea I know, so wouldn’t the type checker still experience an import cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, type checker wouldn't experience an import cycle

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@Gatsik Gatsik Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because type checkers don't run the code

server/gameconnection.py Outdated
Comment on lines 619 to 622
"Desync": GameConnection.handle_desync,
"GameState": GameConnection.handle_game_state,
"GameOption": GameConnection.handle_game_option,
"GameMods": GameConnection.handle_game_mods,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nooo. This is so much worse xD

server/games/custom_game.py
@@ -1,4 +1,3 @@
import asyncio
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I am seeing an unused import warning for this using flake8. It might have been in there for a long time though, like since before we added codacy?

server/games/game.py
@@ -1,9 +1,21 @@
from __future__ import annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh does this add forward references? That's pretty nice, although I'm hoping to migrate to python 3.9 soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this will become the default behavior only in python 3.10

server/lobbyconnection.py Outdated Show resolved Hide resolved
tests/integration_tests/test_oauth_session.py Outdated Show resolved Hide resolved
tests/unit_tests/test_profiler.py Show resolved Hide resolved
@Gatsik
Ignore E226 check
c695b65 
(missing whitespace around arithmetic operator)
@Gatsik
Ignore E241 check
7393b61 
(multiple spaces after ',')
@Gatsik
Except Exception instead of BaseException
2a869c9
@Gatsik
Try to fix W606
57c81da 
(‘async’ and ‘await’ are reserved keywords starting with Python 3.7)
@Askaholic
Copy link
Collaborator

Superceeded by #881

@Askaholic Askaholic closed this Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Askaholic Askaholic Askaholic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Gatsik @Askaholic