Fix request/protocol order of consumption #666
Conversation
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
6a9b635 to
6c22de7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #666 +/- ##
==========================================
+ Coverage 83.38% 85.01% +1.62%
==========================================
Files 39 42 +3
Lines 3918 4437 +519
==========================================
+ Hits 3267 3772 +505
- Misses 651 665 +14 |
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
989eb90 to
4a3f83e
Compare
cameronangliss
force-pushed
the
fix-request-protocol-consume-order
branch
from
f404a69 to
ae0777b
Compare
|
Love this change -- long overdue! Have you written tests to show this works? I'm looking at this and comparing this to Showdown protocol, but might be easier for hsahovic to if add tests (also so if anyone after you messes up, we'll know) |
|
Yes, will get to that hopefully tomorrow, I'll see when I can find time. |
|
@hsahovic this is all set! Passing to you |
src/poke_env/player/player.py
Outdated
| @@ -262,6 +262,9 @@ async def _handle_battle_message(self, split_messages: List[List[str]]): | |||
| :type split_message: str | |||
| """ | |||
| # Battle messages can be multiline | |||
| will_move = False | |||
| from_teampreview_request = False | |||
There was a problem hiding this comment.
i think these two could be named a bit more explicitly, for instance something like should_process_request and should_process_teampreview_request
There was a problem hiding this comment.
How's this: f13727b
| split_messages = protocol or [[f">{battle_tag}"]] | ||
| if request is not None: | ||
| split_messages += [request[1]] | ||
| await self._handle_battle_message(split_messages) # type: ignore |
There was a problem hiding this comment.
my understanding is that the changes in the client are to not treat requests and protocol updates separately, is that correct?
if so, i think this logic should live in Player._handle_battle_message, as the client should be agnostic to implementation details of the battling protocol
additionally, requests should be stored in battle objects instead of the protocol / player objects
There was a problem hiding this comment.
Not exactly. It is to swap the order that they are consumed. I would argue that the logic should be in the client since _handle_battle_message is built to take only one split_messages at a time. I tried to make it so we didn't have to combine the protocol and request as I have done here, but I wasn't able to get that to work unfortunately. The logic inside of _handle_battle_message was fighting back at me hahaha
There was a problem hiding this comment.
Stated simply, this is the change:
BEFORE: we consume messages as they come. Showdown sends the request before the protocol, so we consume request and then protocol.
AFTER: we consume messages as they come, EXCEPT for the request-protocol pair, which we swap the order of upon receiving them.
Refer to the PR description for why this is a good idea
There was a problem hiding this comment.
i see - in this case let's pause on this one for now; i want to take a look at possible alternatives
There was a problem hiding this comment.
Sure thing. One path forward I can see is to do this change and then make an effort towards the ps-client rework that we were talking about the other day. Let me know if you have any other questions.
…request-protocol-consume-order
…request-protocol-consume-order
Fixes #616
Currently we consume the messages in the order they come in, which is request and then protocol message. We really should update the battle object with the protocol first, and then update with the battle request - and now we do! This is better because it prevents us from accidentally overwriting something that is definitely true with our interpretation of the protocol, which currently has bugs. Specifically, this guarantees that we can correctly track Zoroark if it's on our team, and it likely resolves many other bugs that have remained silent up to this point.
NOTE: This diff is minimal but somewhat hacky. I really would have loved to just rework the way the client-server interaction is going on so that the flow of the code could be a lot more natural, but I couldn't figure out how to do it without a gigantic diff. I leave it to a future brave soul to smooth this all out someday.