Added logging, clarified code#219
Added logging, clarified code#219paddybyers merged 4 commits intoably:masterfrom psolstice:RSL6_test
Conversation
mattheworiordan
left a comment
mattheworiordan
left a comment
There was a problem hiding this comment.
LGTM, but need @paddybyers feedback
| import io.ably.lib.types.Param; | ||
| import io.ably.lib.types.PresenceMessage; | ||
| import io.ably.lib.types.ProtocolMessage; | ||
| import io.ably.lib.types.*; |
There was a problem hiding this comment.
I believe @paddybyers is not a big fan of these broad imports, but I'll let him chime in :)
There was a problem hiding this comment.
It was an automatic change by IDEA.
Well I prefer that the imports are explicit but if by default IDEA is going to give warnings about it then I don't see the point in fighting it ... |
|
It gave no warning whatsoever. It just changed it. It will change it again in future if we revert it now. So I don't see the point to fight it too. |
| Log.e(TAG, String.format("%s on channel %s", e.errorInfo.message, name)); | ||
| } catch(AblyException e) { | ||
| Log.e(TAG, "Unexpected exception decrypting message", e); | ||
| Log.e(TAG, String.format("Unexpected exception decrypting message on channel %s", name), e); |
There was a problem hiding this comment.
Will this only be a decrypt exception? Ie should it say "Unexpected exception decoding message" ?
Actually, what exception could occur other than a MessageDecodeException ?
There was a problem hiding this comment.
@paddybyers I saw the same thing, but then realised the previous code was like this before... I would agree we should instead catch MessageDecodeException if that is the exception we expect for this message, and perhaps another generic AblyException catch all?
There was a problem hiding this comment.
Exception other than MessageDecodeException can be thrown only from here
[https://github.com/psolstice/ably-java/blob/RSL6_test/lib/src/main/java/io/ably/lib/types/BaseMessage.java#L105]
And as you can see it's a decryption exception
paddybyers
left a comment
paddybyers
left a comment
There was a problem hiding this comment.
Looks good, just pls have a look at the comment/question
That's also a |
|
Thanks, LGTM |
|
LGTM, thanks |
3 participants
No description provided.