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

Improved battle_against API #657

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

cameronangliss
Copy link
Contributor

No description provided.

@cameronangliss cameronangliss marked this pull request as draft December 5, 2024 08:58
@hsahovic hsahovic force-pushed the master branch 2 times, most recently from 7d8bd5b to 524b047 Compare December 10, 2024 01:36
Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.44%. Comparing base (f458350) to head (9deb368).
Report is 105 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
+ Coverage   83.38%   85.44%   +2.06%     
==========================================
  Files          39       42       +3     
  Lines        3918     4419     +501     
==========================================
+ Hits         3267     3776     +509     
+ Misses        651      643       -8     

@cameronangliss cameronangliss marked this pull request as ready for review December 15, 2024 11:07
@cameronangliss
Copy link
Contributor Author

@hsahovic this one's ready for you!

@@ -692,7 +694,9 @@ async def _ladder(self, n_games: int):
perf_counter() - start_time,
)

async def battle_against(self, opponent: "Player", n_battles: int = 1):
async def battle_against(
Copy link
Owner

Choose a reason for hiding this comment

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

Can you update the docstring?

Copy link
Owner

Choose a reason for hiding this comment

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

Does this make n_battles a required named argument? If so i'd prefer opponent to be either a player or a list of players, and keep n_battles from requiring to be named.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated docstring: 411a469
No need to worry, n_battles is still an optional argument - I'm pretty sure the only requirement is that all the opponents are listed before n_battles is specified, and you need to explicitly say "n_battles=..." in order to specify the integer from the Player objects.

to_id_str(self.username), n_battles, opponent.next_team
),
)
results[opponent.username] = (self.win_rate, opponent.win_rate)
Copy link
Owner

Choose a reason for hiding this comment

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

This should bot be part of this method - reporting results is unrelated to starting battles. Similarly, resetting the stored battles after the battles are done would be counterintuitive and make for a bad API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Here you go: f17c415

@cameronangliss
Copy link
Contributor Author

@hsahovic alright I've attended to your thoughts, let me know what you think! Got some nice improvements in, thanks for the review!

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