Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refacto applicabilité #159

Merged
merged 9 commits into from
Mar 8, 2022
Merged

Refacto applicabilité #159

merged 9 commits into from
Mar 8, 2022

Conversation

mquandalle
Copy link
Collaborator

@mquandalle mquandalle commented Feb 4, 2022 •
edited
Loading

Motivations dans #158

  • Introduction d'une valeur littérale “non applicable” distincte de la valeur “non”
  • Changement des valeurs littérales “non définie” et “non applicable” pour être mieux aligné sur la sémantique JavaScript
  • Ajout de tests unitaires correspondants à l'utilisation sur mon-entreprise avec des cas particuliers (valeurs non applicables ou non définies dans certaines opérations)
  • Ajout d'une passe d'analyse statique pour déterminer les variables qui peuvent être “non applicable” dans leur formule de calcul (ie, on ignore volontairement la désactivation de branche)
  • Modification de l'implémentation de la désactivation de branche :
    • tous les parents sont référencés au niveau de la règle
    • seules les règles “nullables” sont évaluées. Fixes Bug missingVariables ?  #30
    • supprime la logique de traitement particulier des sommes et comparaisons lors de l'évaluation (avec l'inférence isNullable on a une logique plus générale réalisé lors du parse). Évite d'avoir 2 caches distincts pour l'évaluation. Config CI and various fixes #66 (comment)

Prochaines étapes :

  • Inférence complète de type (aligné sur typeof JS), d'unité et de précision pour les nombres décimaux
  • Revoir le comportement des variables null/undefined avec certains mécanismes ? (exemple applicable si: undefined ?) (j'ai reproduit les comportements actuels pour limiter les changements cassants mais il y a matière à débat)

@mquandalle mquandalle force-pushed the refacto-applicability branch 6 times, most recently from 18efce9 to 938d89d Compare February 8, 2022 16:15
@mquandalle mquandalle marked this pull request as ready for review February 8, 2022 17:18
@mquandalle mquandalle force-pushed the refacto-applicability branch 4 times, most recently from da36827 to eedbc83 Compare February 10, 2022 11:19
@mquandalle mquandalle mentioned this pull request Feb 10, 2022
Merged
@mquandalle mquandalle force-pushed the refacto-applicability branch 4 times, most recently from 36843e1 to bc90bf0 Compare February 15, 2022 16:05
@johangirod johangirod mentioned this pull request Feb 18, 2022
Closed
@mquandalle mquandalle force-pushed the refacto-applicability branch 6 times, most recently from ca092d2 to 40ef7d1 Compare March 6, 2022 16:06
@mquandalle
Infer nullability
3c66a56 
Add topological sorting, which should be reusable for complete units
inference.

We had to ignore some specific references to avoid cyclic graphs. I
believe the AST should be reworken to avoid these cycles.
@mquandalle
Re-implement branch disabling
0646f6d 
Using the new infered "isNullable" property
@mquandalle
Export rules dependencies from the main parse function
9989f39 
Remove the duplicated logic in the cyclic dependencies tests
@mquandalle
Re-implement previous behavior in edge cases
1939807 
The introduction literal for not applicable values (distinct from the
`false` literal) introduced several side effects in the mon-entreprise
code base. The commit re-implement previous behavior in the edge cases
and adds several units tests so that mon-entreprise doesn't rely unclear
/ unspecified behavior.
@mquandalle mquandalle mentioned this pull request Mar 7, 2022
Closed
@mquandalle mquandalle force-pushed the refacto-applicability branch 2 times, most recently from 3872f69 to ced4b53 Compare March 7, 2022 06:14
johangirod
johangirod approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@johangirod johangirod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est top ! Ça vaudra le coup de mettre à jour la doc pour expliquer tout ça mais ça peut attendre le merge :)

packages/core/source/parsePublicodes.ts Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
packages/core/source/mecanisms/applicable.ts Outdated Show resolved Hide resolved
packages/core/source/mecanisms/operation.ts Show resolved Hide resolved
packages/core/source/mecanisms/operation.ts Show resolved Hide resolved
packages/core/source/rule.ts Show resolved Hide resolved
packages/core/test/mécanismes/applicable.yaml Outdated Show resolved Hide resolved
packages/core/test/mécanismes/desactivation-branche.yaml Show resolved Hide resolved
packages/react-ui/source/mecanisms/Composantes.tsx Outdated Show resolved Hide resolved
packages/react-ui/source/mecanisms/common.tsx Outdated Show resolved Hide resolved
@mquandalle mquandalle force-pushed the refacto-applicability branch 3 times, most recently from 0aee98d to c62854d Compare March 8, 2022 09:03
@mquandalle mquandalle merged commit 8466ea5 into master Mar 8, 2022
@mquandalle mquandalle deleted the refacto-applicability branch March 8, 2022 10:32
@mquandalle mquandalle mentioned this pull request Mar 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johangirod johangirod johangirod approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug missingVariables ?
2 participants
@mquandalle @johangirod