issue #1107 do not ignore encryptKey and decryptKey exceptions + update readme

[IvanPizhenko]

close #1107
close #1106

[DenBond7]

@IvanPizhenko There are no bugs. Just need to add a few changes.

[DenBond7]

@IvanPizhenko

val key = parseAndNormalizeKeyRings(armored).firstOrNull()
        ?: throw NullPointerException("Keys not found")
    if (key !is PGPSecretKeyRing) throw IllegalArgumentException("Wrong PGPKeyRing")
    return encryptKey(key, passphrase).armor()

And I'd like to see different methods(public) for different input source (String, ByteArray, PGPSecretKeyRing)

[IvanPizhenko]

Do we really need that now? I can see all current usages are with string. Let's add such methods right when they are first time needed. Methods for the PGPSecretKeyRing are available, but currently private. When they are needed to be called externally, we can make them public.

[IvanPizhenko]

NullPointerException? I think IllegalArgumentException in more suitable, because you are actually never getting null value as input parameter, and error is better to be about the actual input parameter rather than about result of the some intermediate transformation which is intentionally designed so it can result a null value.

[DenBond7]

The same for decryptKey

[DenBond7]

Including armor

[DenBond7]

@IvanPizhenko
It'll be better to don't armor a key right here. We called this method encryptKey but actually, we do encryption and armor, but I just expect encryptKey due to the method name. Please don't use armor here. As for me will be better to use PgpKey.encryptKey(private, passPhrase).armor() than PgpKey.encryptKey(private, passPhrase).

[IvanPizhenko]

The current uses of this provide key as armored string on the input and expect armored output. That's why it is done this way. I suggest to have overloaded methods with output type matching to the input type. But I suggest to implement them only when they are actually required.

[DenBond7]

@tomholub Sorry to disturb

1. Could you clarify is it the same as we discussed at Kotlin code style consistency with other codebases #1122?

- if (key is PGPSecretKeyRing)
-      return key
-    else
-     throw IllegalArgumentException("Key is not a secret key")
+ return (key as? PGPSecretKeyRing)?:throw IllegalArgumentException("Key is not a secret key")

2. And what about braces to 'if' and 'else' statements?

- if (key is PGPSecretKeyRing)
-      return key
-    else
-     throw IllegalArgumentException("Key is not a secret key")
+ if (key is PGPSecretKeyRing) {
+    return key
+ } else {
+    throw IllegalArgumentException("Key is not a secret key")
+ }

[tomholub]

if/else statements do not have to have braces if on one line. But in that case it's only permitted to use for "consistent" result, eg like a ternary statement return if (c) 1 else 2 or val x = if (c) 1 else 2. Cannot mix return and throw on one line or without braces.

if/else statements MUST have braces if multiline. I wish there was some ktlint rule for this (maybe there is?)

// good
val x = if(c) 1 else 2

// good
return if(c) 1 else 2

// bad
if (key is PGPSecretKeyRing)
      return key
    else
     throw IllegalArgumentException("Key is not a secret key")

// good
if (key is PGPSecretKeyRing) {
  return key
} else {
  throw IllegalArgumentException("Key is not a secret key")
}

// also good but depends on style? I prefer this but other people prefer the above. Not much difference
if (key is PGPSecretKeyRing) {
  return key
}
throw IllegalArgumentException("Key is not a secret key")

This is also good. Maybe the best - I find it succinct and clear:

// good
return (key as? PGPSecretKeyRing) ?: throw IllegalArgumentException("Key is not a secret key")

// if it doesn't fit on one line, a `?: throw` on next line is also good
return (key as? PGPSecretKeyRing)
  ?: throw IllegalArgumentException("Key is not a secret key")

I think everybody will have a little different idea, but I think this is a decent compromise.

[DenBond7]

👍

[DenBond7]

it looks good to me. Approved