Release 1.1 with specific Error types.

[alexruperez]

Goals ⚽

• Set kommand error closure specifying Error type.

---

This change is 

[codecov]

Codecov Report

Merging #15 into develop will decrease coverage by 0.9%.
The diff coverage is 42.1%.

@@             Coverage Diff             @@
##           develop      #15      +/-   ##
===========================================
- Coverage    85.97%   85.06%   -0.91%     
===========================================
  Files           10       10              
  Lines          670      683      +13     
===========================================
+ Hits           576      581       +5     
- Misses          94      102       +8

Impacted Files	Coverage Δ
Source/Dispatcher.swift	80.76% <0%> (ø)	⬆️
Source/Kommander.swift	60% <0%> (ø)	⬆️
Major/ViewController.swift	100% <100%> (ø)	⬆️
Source/Kommand.swift	80% <42.85%> (-5.3%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6f893e...beee0d1. Read the comment docs.

[igzrobertoestrada]

Como developer que se mueve entre dos plataformas estoy diametralmente en contra del renaming de los métodos make() y execute() porque tengo que hacer el doble de esfuerzo para recordar cómo se usa la librería en iOS y también su hermana en Android.

Segundo y hasta casi más importante, hacer un renaming sin aviso de los métodos es un cambio rompiente y traumático del contrato que a mí como usuario me ofrece la librería teniendo que cambiar todas las referencias a los métodos modificados forzándome a tener que vivir en una versión desfasada de la librería.

Voto en contra del renombrado

En cuanto a poder especificar el tipo de error que esperamos recibir puede ser interesante, pero si espero recibir distintos tipos de error no me vale de nada porque no puedo hacer pattern matching para tomar decisiones con respecto a cada tipo de error.

Si se me sigue ofreciendo la capacidad que antes tenía de recibir un Error y se añade esta nueva posibilidad, voto a favor. Pero si no es así, no.

[alexruperez]

Si, si, en el tema de los errores se mantiene el método antiguo donde puedes seguir haciendo pattern matching, esto es para cuando sabes que vas a recibir uno y solo un tipo de Error (por ejemplo cuando sabes que va a ser un NetworkServiceError o un InteractorError concreto y no quieres castear continuamente el error)

En cuanto al cambio de nombre, las librerías open source de sugar syntax tienden a comandos y ordenes mas sencillos y rápidos de escribir, por eso este cambio, no obstante, seguimos semantic versioning y esta versión cambiará a 1.1 por el cambio de nombre.

Voto apuntado, gracias por la review! 👍

[juantrias]

Reviewed 37 of 41 files at r1, 1 of 1 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alexruperez, @RobertoEstrada, @IGZCarlosManzanas, and @IGZcarlosgarcia)

a discussion (no related file):
No me gusta el rename, de hecho me quedaría con los nombres originales de makeKommand, onSuccess, onError y execute. Me parece mucho más semántico makeKommand ya que es un factory method que devuelve un Kommand. Con autocomplete es igual de rápido escribir makeKommand que do. Para conseguir un código más conciso se puede definir una función global makeKommand() que haga Kommander.makeKommand() como hemos hecho en algún proyecto. Además do es una palabra reservada y aunque se pueda usar poniendo las comillas intentaría evitarlo.

---

a discussion (no related file):
Creo que se deberían crear dos PRs separadas para el rename y para el error() tipado ya que no tienen nada que ver y son dos debates distintos

---

---

Source/Kommand.swift, line 114 at r3 (raw file):

        self.errorClosure = {
            guard let reason = $0 as? Reason else {
                return

+1 Al error() tipado. Si seguimos la política de encapsular los errores por capas nos ahorra mucho casting innecesario en la vista. Lástima que el throws de Swift no sea tipado y no se pueda asegurar en tiempo de compilación que lo que se ejecuta dentro del make lance un error del tipo esperado. Esto puede dar problemas con la implementación actual ya que si se lanza otro tipo de error no se llamará al bloque de error. Y esto es lo típico que se nos puede colar en un refactor y no darnos cuenta hasta mucho tiempo después cuando veamos que no se pintan los errores.

Como lo más habitual es pintar un mensaje genérico para todos los errores menos algún caso específico propongo que siempre se llame al bloque de error y si no es del tipo esperado se pase nil, igual que en el retry.

[alexruperez]

@igzrobertoestrada @juantrias he deshecho el rename, me han gustado vuestros argumentos.

En cuanto a lo del error, he añadido un assert para que nos demos cuenta durante el desarrollo de que no hemos controlado algún error, pero si llegase a producción no cause un crash o comportamiento extraño. Esto lo he hecho para no tener que hacer unwrap cada bloque de error. Si aquí https://github.com/intelygenz/Kommander-iOS/pull/15/files#diff-7cd8704470fc0c9a336dfc5e14e8c4f8R111 pongo Reason?, aunque lancemos un error concreto y lo especifiquemos, habrá que hacer unwrap siempre, creedme, lo he probado y a nivel de usabilidad de la lib, es un rollo.

Si me dais el ok la lanzo. Gracias de nuevo por la review chicos. 😃

[juantrias]

Reviewed 36 of 36 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alexruperez, @RobertoEstrada, @IGZCarlosManzanas, and @IGZcarlosgarcia)

---

Source/Kommand.swift, line 114 at r4 (raw file):

        self.errorClosure = {
            guard let reason = $0 as? Reason else {
                assertionFailure("Unexpected error thrown. \(Reason.self) expected, \($0.debugDescription) thrown.")

+1