-
Notifications
You must be signed in to change notification settings - Fork 61
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
Tournament game service #921
base: develop
Are you sure you want to change the base?
Conversation
@@ -415,7 +415,8 @@ async def test_handle_action_GameOption( | |||
assert game.max_players == 7 | |||
# I don't know what these paths actually look like | |||
await game_connection.handle_action("GameOption", ["ScenarioFile", "C:\\Maps\\Some_Map"]) | |||
assert game.map_file_path == "maps/some_map.zip" | |||
assert game.map_name == "some_map" | |||
assert game.map_scenario_path == "C:/Maps/Some_Map" |
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.
since game is a mock the setter is not invoked and map_file_path is not set by the setter
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.
Sort of half reviewed it. Biggest thing is please undo the indentation changes. It looks like you might have an autoformatter or something that is changing those.
def wait_launched(self, param): | ||
pass | ||
|
||
def wait_hosted(self, param): | ||
pass |
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.
Do we need these in the base class? They only make sense for automatch games.
@@ -516,23 +518,23 @@ def get_player_mean(player: Player) -> float: | |||
if game_options: | |||
game.gameOptions.update(game_options) | |||
|
|||
mapname = re.match("maps/(.+).zip", map_path).group(1) | |||
map_name = re.match("maps/(.+).zip", map_path).group(1) |
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.
Can we refactor the regex?
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.
Where to?
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.
How about just making use of the properties you added to the Game class? The map_file_path is already being set on line 532.
map_name = game.map_name
make_game_options: Callable[[Player], GameLaunchOptions] | ||
async def launch_server_made_game( | ||
self, | ||
game: Game, |
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.
Could make an intermediate class AutomatchGame
|
||
def is_confirmation_overdue(self): | ||
time_passed = datetime.utcnow() - self.created_time | ||
return time_passed.seconds > self.response_time_seconds + 5 |
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.
Why hardcode +5 here? Just set the response time to 35 seconds?
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 did give the client 5 more seconds to answer because of network delay etc.
What do you mean by just set the response time higher? That would also cause the client to display a longer time to the user.
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.
Ok I see. I didn't catch that the response time is sent to the client.
6bb54ed
to
f5442eb
Compare
f5442eb
to
a93560d
Compare
@@ -0,0 +1,766 @@ | |||
[*] | |||
charset = utf-8 |
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 need to look into this editorconfig stuff. Would probably be best to do it in a separate PR. This file also looks like it has way too much stuff in it. I would want to trim out anything we don't need.
@@ -141,6 +140,10 @@ async def _handle_rating_queue(self) -> None: | |||
async def _rate(self, summary: GameRatingSummary) -> None: | |||
assert self._rating_type_ids is not None | |||
|
|||
if summary.rating_type is None: | |||
self._logger.debug(f"Not processing game {summary.game_id} since it is not rated.") |
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.
We don't use f-strings in log messages.
self._logger.debug(f"Not processing game {summary.game_id} since it is not rated.") | |
self._logger.debug("Not processing game %s since it is not rated.", summary.game_id) |
Closes #675