Validateur NeTEx : quelques metadata #4160
Conversation
Traquons le nombre de retries et la durée d'une validation.
There was a problem hiding this comment.
Bien joué !
J'ai fait des suggestions sur le style ; sur le fond rien à redire !
J'aurais eu un peu plus de temps ce matin, j'aurais fait tourner le coverage (https://github.com/etalab/transport-site?tab=readme-ov-file#measuring-test-coverage), pour avoir une idée plus précise ce qui est couvert par les tests ; si jamais ça te tente ça peut être intéressant !
| | :failed | ||
| | {:successful, binary(), integer()} | ||
| | {:warning, integer()} | ||
| | {:failed, integer()} |
There was a problem hiding this comment.
Je me suis demandé initialement si ce tuple où on ajoute un entier en bout de chaîne ne commençait pas à devenir trop long, ne risquait pas de poser de problème quand on fait évoluer cette structure etc.
Mais au final ma compréhension du code actuel est que c'est une structure éphémère non persistée en base et qui pourra donc être revue sans souci (ex: besoin de passer à une map avec des clés plus explicites).
Je consigne juste cette note pour ma compréhension !
There was a problem hiding this comment.
En effet, c'est bien ça.
Disons que c'est une manifestation de mon habitude de manipuler des types algébriques en entrée ou sortie de toute fonction et d'apprécier d'avoir une documentation implicite via les types des embranchements possibles.
Le fait que l'entier que je rajoute dans cette PR ne soit pas identifié comme une durée en seconde est une limite de cette syntaxe, et on pourrait arguer que le spec ainsi obtenu devient dangereusement verbeux.
(I miss my algebraic types.)
|
|
||
| "failed" -> | ||
| :failed | ||
| {:failed, get_elapsed(response)} | ||
|
|
||
| _ -> | ||
| :unexpected_validation_status | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Pour ci-dessous : j'ai tendance à préférer des appels en bang "!" qui lèvent une erreur, pour réduire le code, et du coup réduire les potentialités de bug dans le code qui analyse les retours.
Par exemple ici j'aurais opté pour quelque chose comme:
defp get_elapsed!(response) do
created_at = get_datetime!(response, "created_at")
updated_at = get_datetime!(response, "updated_at")
DateTime.diff(updated_at, created_at)
endEt (voir https://hexdocs.pm/elixir/1.17.2/Date.html#from_iso8601!/2)
defp get_datetime!(map, key) do
map
|> Map.fetch!(key)
|> DateTime.from_iso8601!()
endC'est d'ailleurs en phase avec le fait que Map.fetch! va de toute façon lever une exception en cas de clé manquante, et adopte donc plutôt un style "fail fast" aussi.
(Et du coup, gérer l'erreur dans l'appelant de get_elapsed!)
Après ça reste aussi une opinion, mais perso je trouve que ça fait au final beaucoup moins de code sur une codebase quand c'est applicable (il faut que l'appelant puisse intercepter l'erreur, par contre !)
There was a problem hiding this comment.
il faut que l'appelant puisse intercepter l'erreur, par contre !
Oui, et tu peux avoir des cas où la clef est manquante/invalide sans que ça soit grave, pas forcément la responsabilité du code appelant.
En effet si j'étais cohérent j'aurais pas utilisé Map.fetch!. En pratique je peux faire confiance au fait que le datetime est bien formatté. Ce cas d'erreur spécifique m'ennuie.
Je dois revoir cette partie de toute façon, j'ai identifié un problème en retestant avec la validation OnDemand.
There was a problem hiding this comment.
Et (voir https://hexdocs.pm/elixir/1.17.2/Date.html#from_iso8601!/2)
Ce helper est fourni pour Date mais pas pour DateTime ; je vais l'émuler.
There was a problem hiding this comment.
J'imagine que c'est lié au fait que DateTime.from_iso8601 renvoie et le datetime et l'offset, et donc qu'il faudrait l'interpréter également (ce que je ne fais pas, je fais l'hypothèse que l'offset est constant).
Une utilisation naïve des commandes listées dans le readme échoue par un "Process killed" chez moi. Cela dit je suis relativement confiant quant à la couverture des tests. Le plus gros souci à mon goût est surtout que malgrè la présence des typespec, et à cause de l'utilisation d'un mock sur ce wrapper dans les tests du code appelant, un changement du client peut casser le code appelant sans que ça saute aux yeux dans la CI. |
|
Il est possible que le timeout implémenté dans #4157 ne fonctionne pas non plus en pratique à cause de ça. Ne pas merger donc. |
La mesure du temps passé ne fonctionnait pas comme prévu : le champs updated_at n'est en effet pas actualisé chez enRoute. Pour mesurer le temps passé à valider un fichier NeTEx il faut plutôt considérer started_at et ended_at. Ce dernier n'est pas naturellement pas renseigné pour le cas :pending. Le timeout est désormais implémenté par le nombre de retries => 100 retries est donc désormais la limite haute.
C'est fait. |
Sur une suggestion d'Antoine, cette PR garde trace du temps passé et du nombre de retries pour une validation NeTEx.
Voir #4153.