-
Notifications
You must be signed in to change notification settings - Fork 10
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
Improve SDK exceptions #383
Conversation
5fdf1d7
to
af8a1c1
Compare
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.
Looks good.
The sense behind the exception hierarchy is lost, I'm afraid (I didn't program that).
Maybe we should add comments to the new exceptions describing what they are for and how the expected behavior of the server and client should be if such an exception happens? To make sure that the sense is not lost again.
try { | ||
ReservationManager.redeemReservationCode(source, packet.reservationCode) | ||
} catch(e: RescuableClientException) { | ||
source.send(ProtocolErrorMessage(packet, e.message)) |
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.
So the exceptions are now sent here to the client instead having the handleInvalidMove method for that, right?
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.
no, this change has nothing to do with moves. Please look into the individual commits for coherence.
81c737f
to
b7e6bf0
Compare
Was mir allerdings noch ein Rätsel ist: ein Client stoppt bei einer
RescuableClientException
sofort - wozu ist sie dannRescuable
? Bei nem falschen Reservierungscode oder so kann der Client doch weiterhin verbunden bleiben?FYI some visualizations: