[small] Feature/add tests#10
Conversation
| def __init__(self, player_ids: Optional[list[PlayerId]] = None) -> None: | ||
| super().__init__() | ||
| if player_ids is None: | ||
| player_ids = [PlayerId(i) for i in range(3)] |
There was a problem hiding this comment.
if these repo makers are to be used for anything other than tests (e.g., create a new game), maybe we don't want the player counter to start at zero?
There was a problem hiding this comment.
The idea is not to use them for anything other than tests. Why would we not want to start at 0? The player number will never be displayed to the player it is just for the back end. The player will see their name
| def make_quick( | ||
| cls, n_normal_assets: int = 3, player_ids: Optional[list[PlayerId]] = None, bus_repo: Optional[BusRepo] = None | ||
| ) -> AssetRepo: | ||
| return cls(player_ids=player_ids, bus_repo=bus_repo).add_n_random(n_normal_assets).make() |
There was a problem hiding this comment.
what are normal assets?
There was a problem hiding this comment.
Normal assets are not ice cream buses
| return self | ||
|
|
||
| def _get_random_player_id(self) -> PlayerId: | ||
| return safe_random_choice(self.player_ids) if safe_random_choice([True, False]) else PlayerId.get_npc() |
There was a problem hiding this comment.
this method seems overcomplicated and gives preference to the npc. Why not just appending the npc id to the player_ids?
There was a problem hiding this comment.
If you want to generate 10 random assets, then probably half of them will belong to the npc
| return AssetRepo | ||
|
|
||
| def _pre_make_hook(self) -> None: | ||
| ice_cream_buses = [bus for bus in self._bus_repo if bus.is_ice_cream_bus] |
There was a problem hiding this comment.
finding ice cream buses could be a method in BusRepo
| self.player_ids = player_ids | ||
| self.bus_ids = bus_ids | ||
|
|
||
| def _get_random_player_id(self) -> PlayerId: |
There was a problem hiding this comment.
same as before (overcomplicated method and gives preference to npc), and can we somehow avoid this duplicated code?
| def _pre_make_hook(self) -> None: | ||
| # Connect islands before making the repo | ||
| mentioned_buses = {t.bus1 for t in self.dcs} | {t.bus2 for t in self.dcs} | ||
| islanded_buses = [bus for bus in self.bus_ids if bus not in mentioned_buses] |
There was a problem hiding this comment.
another good method to include in BusRepo: islanded buses
There was a problem hiding this comment.
That cannot be in the bus repo because then the bus repo would have to know about the transmsision repo
| from tests.utils.repo_maker import AssetRepoMaker, BusRepoMaker | ||
|
|
||
|
|
||
| class TestPlayers(TestCase): |
There was a problem hiding this comment.
you are testing more than just the players, right? check the class name
There was a problem hiding this comment.
it seems you copied the methods from test_assets.py and forgot to delete them after
|
|
||
| def __post_init__(self) -> None: | ||
| assert self.bus2 > self.bus1 | ||
| assert self.bus2 > self.bus1, f"bus2 must be greater than bus2. Got {self.bus2} and {self.bus1}" |
There was a problem hiding this comment.
the string has a typo, bus2 is repeated
2 participants
Uh oh!
There was an error while loading. Please reload this page.