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.

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

Add a situation (and foldedSteps) migration function #39

Merged
merged 13 commits into from
May 16, 2024

Conversation

bjlaa
Copy link
Collaborator

@bjlaa bjlaa commented May 14, 2024

No description provided.

@bjlaa bjlaa requested review from EmileRolley and Clemog May 14, 2024 13:24
@bjlaa bjlaa self-assigned this May 14, 2024
@Clemog
Copy link
Contributor

Clemog commented May 14, 2024

Je me charge de tester l'implé dans le site de NGC pour tester ?
Est ce qu'on ajoute ici des tests unitaires ?

Copy link
Collaborator

@florianpanchout florianpanchout left a comment

Choose a reason for hiding this comment

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

Est-ce que cette fonction ne devrait pas aussi détecter les règles qui ne sont pas valides ? (mais pas incluses dans les instructions de migration). En plus de la situation et des foldedSteps "vraiment" propre, on pourrait retourner un array contenant les règles non gérées.

Aussi avoir ce safeGetSituation dans ce repo permettrait d'exporter un autre utils safelyAddSituation qui prendrait comme props une situation, l'objet d'option de addSituation, un engine et les règles. Ensuite il n'y aurait plus qu'à utiliser exclusivement cette fonction pour ajouter à la situation.

@bjlaa
Copy link
Collaborator Author

bjlaa commented May 15, 2024

Je me charge de tester l'implé dans le site de NGC pour tester ?

@Clemog j'ai testé de mon côté en local en buildant tools et en filant son path dans le package.json du site ; j'ai l'impression que tout roule, même avec mes simulations de septembre 2023, mais je suis preneur de ton avis et de ton oeil de lynx ici si tu sais tester aussi en local !

// We check if the non supported ruleName is a key to migrate.
// Ex: "logement . chauffage . bois . type . bûche . consommation": "xxx" which is now ""logement . chauffage . bois . type . bûches . consommation": "xxx"
if (Object.keys(migrationInstructions.keysToMigrate).includes(ruleName)) {
const result = handleSituationKeysMigration({
Copy link
Collaborator

Choose a reason for hiding this comment

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

const { situationMigrated, foldedStepsMigrated } = non ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Je crois que ce n'est pas possible car on utilise situationMigrated, foldedStepsMigrated dans les arguments

@EmileRolley
Copy link
Collaborator

Hmm, pourquoi vous voulez ajouter la gestion des migrations dans ce dépôt ? Pour l'instant, il me semble que c'est propre à NGC la notion de migration non ?

@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

Hmm, pourquoi vous voulez ajouter la gestion des migrations dans ce dépôt ? Pour l'instant, il me semble que c'est propre à NGC la notion de migration non ?

On s'est posé la question de le mettre dans nosgestesclimat-scripts

Ça peut etre utile pour d'autres projets, c'est assez indépendant de NGC finalement donc j'ai poussé pour que ce soit ici

Copy link
Collaborator

@EmileRolley EmileRolley left a comment

Choose a reason for hiding this comment

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

Par soucis d'homogénéité ce serait bien si vous pouviez adopter les même conventions que pour le reste de la lib. Je pense également qu'il faudrait le séparer dans un module à part et pas exposer les fonction à la racine (comme pour /optims et /compilation).

@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

On a besoin de cette fonction dans 2 dépôts, ça me semblait le plus pertinent

Pourquoi tu verrais ça vraiment très spécifique à NGC ?

@EmileRolley
Copy link
Collaborator

Hmm, pourquoi vous voulez ajouter la gestion des migrations dans ce dépôt ? Pour l'instant, il me semble que c'est propre à NGC la notion de migration non ?

On s'est posé la question de le mettre dans nosgestesclimat-scripts

Ça peut etre utile pour d'autres projets, c'est assez indépendant de NGC finalement donc j'ai poussé pour que ce soit ici

Oui c'est vrai que ça peut être utile à d'autre projet. Dans ce cas il faudrait créer un module à part entière et documenter le cas d'usage etc..

@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

Ah yes effectivement, on n'a pas de doc, c'est pas ouf (j'ai l'impression que c'est pas ton kiff @bjlaa 😇)

@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

Je pense également qu'il faudrait le séparer dans un module à part et pas exposer les fonction à la racine (comme pour /optims et /compilation).

C'est à dire ?

@EmileRolley
Copy link
Collaborator

Je pense également qu'il faudrait le séparer dans un module à part et pas exposer les fonction à la racine (comme pour /optims et /compilation).

C'est à dire ?

Les exposer dans un sous-chemin pour les importer depuis @publicodes/tools/migration :

tools/package.json

Lines 18 to 33 in ee4e38a

"exports": {
".": {
"import": "./dist/index.js",
"require": "./dist/index.cjs",
"types": "./dist/index.d.ts"
},
"./optims": {
"import": "./dist/optims/index.js",
"require": "./dist/optims/index.cjs",
"types": "./dist/optims/index.d.ts"
},
"./compilation": {
"import": "./dist/compilation/index.js",
"require": "./dist/compilation/index.cjs",
"types": "./dist/compilation/index.d.ts"
}

Vous pouvez regarder ce qui est fait pour optims et compilation. Leurs index.ts ne sont pas exposés dans l'index racine et cela permet également de rajouter la documentation relative au module : https://github.com/publicodes/tools/blob/main/source/optims/index.ts

@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

Ok merci je regarde ça

@Clemog Clemog requested a review from EmileRolley May 15, 2024 15:34
@Clemog
Copy link
Contributor

Clemog commented May 15, 2024

Comment on peut vérifier l'affichage et tout ?

@EmileRolley
Copy link
Collaborator

Comment on peut vérifier l'affichage et tout ?

En générant la doc (yarn run docs) ?

@Clemog
Copy link
Contributor

Clemog commented May 16, 2024

@EmileRolley on merge car on a besoin d'utiliser la fonction en P0, on peut revenir dessus sans souci :)

@Clemog Clemog force-pushed the add-situation-migration-script branch from 435419a to 0c8371c Compare May 16, 2024 08:07
@Clemog Clemog merged commit 0353d2c into main May 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants