Bugfixes sur next #1291
Bugfixes sur next #1291
Conversation
|
🚀 La branche est déployée ! https://deploy-preview-1291--syso.netlify.app Voir aussi : version anglaise | site publicodes |
En attendant d'avoir un type propre pour la valeur Non Applicable
…les règles imbriquées
johangirod
force-pushed
the
bugfix-next
branch
from
3f0e66d
to
d418f2b
Compare
| @@ -181,7 +182,43 @@ export function evaluateRule<DottedName extends string = string>( | |||
| const rule = engine.getParsedRules()[dottedName] as RuleNode & { | |||
| dottedName: DottedName | |||
| } | |||
|
|
|||
| // HACK while waiting for applicability to have its own type | |||
There was a problem hiding this comment.
Peux-tu me ré-exposer dans quels cas la définition isApplicable = nodeValue !== false ne fonctionne pas ?
There was a problem hiding this comment.
Tout simplement le cas ou j'ai une règle booléenne qui vaut false mais qui est applicable. Si je veux ne pas afficher les champs qui ne sont pas applicable (logique) mais afficher les champs booléens dont la valeur est false, je suis obligé d'avoir un type spécifique pour l'applicabilité.
There was a problem hiding this comment.
Ah oui ok ! On pourrait utiliser null comme représentation JavaScript du type Publicodes Non applicable ? Et undefined pour Manquante ?
There was a problem hiding this comment.
On pourrait. Mais il faudrait se méfier de la différence entre { nodeValue: undefined } et { }
Après, on verra ça dans une prochaine PR. On peut aussi se baser sur des types qui implémentent fantasy-land pour nodeValue, ce qui permettrait d'écrire des mécanisme générique plus facilement. A voir si ça ne pose pas trop de problèmes de perfs ceci dit...
There was a problem hiding this comment.
Je suis allé un peu vite pour l'équivalence undefined~Manquante pour le problème que tu dis, mais aussi et surtout parce qu'une variable manquante peut quand même avoir une valeur. C'est un type produit (contrairement au type somme non applicable), donc on aurait plutôt type Node<T> = {isMissing: boolean, value: T | null}.
En revanche l'utilisation de null pour représenter non applicable me paraît une bonne idée. Je en connais pas fantasy-land, mais si on définit les opérations de base x + [non applicable] = x et x × [non applicable] = [non applicable] en se basant sur null pour l'implémentation, on n'a plus besoin de le faire dans barème, grille, etc. Tu penses qu'on gagnerait à avoir un niveau d'abstraction supplémentaire ?
There was a problem hiding this comment.
Tu penses qu'on gagnerait à avoir un niveau d'abstraction supplémentaire ?
Je ne sais pas trop dire. L'avantage, c'est qu'on pourrait écrire les mécanismes de la même manière, peu importe que la source des données soit une variable de type T ou Maybe, Temporal. Par ailleurs, on pourrait utiliser des structure de donnée fonctionnelle comme Validation pour les remontée d'erreur lors du parsing, de l'évaluation ou du typage (pour ne pas interrompre l'execution). Pour les missingVariables, il semblerait qu'on pourrait utiliser writer monad.
Il semblerait que la librairie https://gcanti.github.io/fp-ts/ soit vraiment top pour cela.
L'autre avantage c'est qu'on aurait pas à réecrire tout un tas de helper comme dans temporal.ts : on pourrait utiliser les fonctions standard de la lib.
L'inconvénient, c'est l'ajout d'une abstraction pas forcément familière.
Avec deux gros points d'interrogation :
- Est-ce qu'il y aurait un impact significatif vis à vis des performances ?
- Est-ce qu'il serait possible d'utiliser "un peu" de fonctionnel sans devoir absolument tout changer d'un coup ?
Probablement que le jeu n'en vaut pas la chandelle... mais à garder dans un coin de la tête et pourquoi pas experimenter un peu quand on aura implémenté #1258
There was a problem hiding this comment.
Je vais regarder fp-ts qui à l'air chouette effectivement, mais j'ai l'impression que ça concerne uniquement le typage ? Ça serait utile pour avoir des types plus expressifs (trivialement préférons Option<T> à T | null), mais on pourrait choisir la représentation "runtime" que l'on veut.
There was a problem hiding this comment.
Mais à la base mon idée c'était que remplacer toutes les utilisations de false par null c'était plutôt plus simple que ce hack. (pas bloquant pour merger)
There was a problem hiding this comment.
Mais à la base mon idée c'était que remplacer toutes les utilisations de false par null c'était plutôt plus simple que ce hack. (pas bloquant pour merger)
null encode déjà pour 'missing'. Si on change sa sémantique, ça risque d'être pas mal de modif à faire un peu partout.
Je suis allé un peu vite pour l'équivalence undefined~Manquante pour le problème que tu dis, mais aussi et surtout parce qu'une variable manquante peut quand même avoir une valeur. C'est un type produit (contrairement au type somme non applicable), donc on aurait plutôt type Node = {isMissing: boolean, value: T | null}.
Il ne faut pas confondre 'liste des missings pour le calcul' et la valeur 'missing'. La valeur 'missing' a une sémantique différente de non applicable, elle n'est pas "recessive", mais "dominante". Si un terme d'une somme est 'missing' alors l'ensemble de la somme a pour valeur 'missing'.
Il faut donc bien 2 types scalaires distincts : Manquante et Non Applicable
| @@ -271,7 +271,7 @@ export function Leaf( | |||
| !node.name.includes(' . ') && | |||
| rule.virtualRule | |||
| if (inlineRule) { | |||
| return <Explanation node={rule} /> | |||
| return <Explanation node={engine?.evaluateNode(rule)} /> | |||
There was a problem hiding this comment.
Pourquoi on a besoin d'ajouter un engine.evaluateNode spécifiquement ici ?
En fait mon intuition c'est qu'il faudrait systématiser cet appel (et donc le faire au niveau du composant Explanation) afin de différencier un appel depuis l'extérieur (ici pour afficher les explications) d'un appel à l'intérieur d'un calcul, en particulier pour formater différement le résultat. cf. #1290 qui est sur cette idée aussi.
There was a problem hiding this comment.
Hmmmm, je ne suis pas sûr qu'il faille systématiser l'appel à evaluateNode pour l'évaluation.
Dans l'absolu, je suis plutôt pour, ça permet d'évaluer toutes les valeurs, et de tout afficher.
Mais l'inconvénient c'est qu'on peut potentiellement se retrouver avec des évaluations qui crée des boucles... qui ne sont pas présente dans le calcul (car leur cas n'arrive jamais au runtime).
En attendant, on a pas forcément besoin de l'appliquer spécifiquement ici, on pourrait se contenter de node.explanation || rule. C'est sans doute mieux d'ailleurs, pour la raison évoquée ci-dessus.
Maintenant pour toutes les règles, y compris quand ce ne sont pas des sommes
johangirod
force-pushed
the
bugfix-next
branch
from
87bde13
to
bba2371
Compare
|
Ok pour merger next 🐱🏍 |
|
Top ! 🎉 |
Corrige les problèmes listés dans #1285