ChannelOptions.withCipherKey + tests#513
Conversation
amihaiemil
changed the title
paddybyers
left a comment
paddybyers
left a comment
There was a problem hiding this comment.
LGTM, but a couple of comments.
| public Object cipherParams; | ||
|
|
||
| /** | ||
| * Are these options encrypted or not? |
There was a problem hiding this comment.
"Whether or not this ChannelOptions is encrypted"
I don't like a question here.
| if(cipher != null) return cipher; | ||
| return (cipher = Crypto.getCipher(this)); | ||
| if(!this.encrypted) { | ||
| return null; |
There was a problem hiding this comment.
I agree with the change to add braces, but why the insistence on using this ?
There was a problem hiding this comment.
@paddybyers I use this. all the time when dealing with class variables. It just seems cleaner to me and its clear from the start that it is a class attribute. I don't like relying on colours :)
I also do it automatically... in previous projects, the build would fail if this, was not used.
There was a problem hiding this comment.
Personally I don't like that. I prefer using this where necessary for disambiguation, but not unconditionally.
| fail("init0: Unexpected exception instantiating library"); | ||
| } finally { | ||
| if(ably1 != null) | ||
| ably1.close(); |
There was a problem hiding this comment.
@paddybyers braces are already added. You commented on the 3rd commit (3482631), but the latest is the 4th (d04cc19) where the braces are added everywhere.
|
@paddybyers please see my answers :) |
|
@paddybyers so can we merge it? :D |
Yes, if it's approved, then it can be merged. |
|
@paddybyers I'm just asking because I don't have the rights to merge PRs :) Thanks! |
2 participants
PR for #492
Changes:
getCipher()(we should avoid inlineifs)withChiperKeyas per spec and deprecatedfromCipherKey.RealtimeCryptoTest:ifs;txandrxtosenderandreceiverrespectively