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

Update parser_and_mv to remove A1 reading and use schemas.yaml #222

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

issam71100
Copy link

updates the parser_and_mv script to eliminate the dependency on reading from cell A1. Instead, it directly utilizes the already generated schemas.yaml file, incorporating the filename and sheet name.

@romainfd
Copy link
Collaborator

Les tests ne passent pas et ça supprime tous les XML car la partie Java ne fonctionne pas
image

@romainfd romainfd added the WIP Work In Progress: the branch is still under active development and not stable / in a working state label Dec 30, 2024
@issam71100 issam71100 force-pushed the feature/parser/remove-a1-cell branch 2 times, most recently from d944532 to a6eec0c Compare January 2, 2025 09:51
@issam71100 issam71100 force-pushed the feature/parser/remove-a1-cell branch 2 times, most recently from 44d9849 to 3cb5ca4 Compare January 9, 2025 17:19
- update parser_and_mv remove A1 reading and use schemas.yaml
- fix xml generation and technical tests
@issam71100 issam71100 force-pushed the feature/parser/remove-a1-cell branch from 3cb5ca4 to f6672d3 Compare January 9, 2025 17:30
Copy link

github-actions bot commented Jan 9, 2025

There is no coverage information present for the Files changed

Total Project Coverage 63.02% 🍏

Copy link
Collaborator

@bou3108 bou3108 left a comment

Choose a reason for hiding this comment

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

Quelques remarques et questions pour le suivi, mais ça me paraît tout à fait bon (et tests OK)

@@ -62,6 +62,11 @@ jobs:
# We specifically only remove FOLDERS with the exception of a couple manually created ones
run: find . -mindepth 1 -maxdepth 1 -type d ! -name 'builders' ! -name 'config' ! -name 'custom' ! -name 'edxl' ! -name 'exception' ! -name 'report' -exec rm -r {} +

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je passe peut-être à côté de l'enjeu : pourquoi a-t-on remonté ce bloc ?

Copy link
Author

Choose a reason for hiding this comment

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

Car le schema.yaml est généré dans la fonction output_schemas_yaml, et il doit avoir été généré avant pour être utilisé dans la fonction parser_and_mv du workflow.py

Comment on lines +12 to +14
import com.hubsante.model.{{ .package }}.{{ .rootElement | title }};{{ if ne .rootElement "distributionElement" }}
import com.hubsante.model.{{ .package }}.{{ .rootElement | title }}Wrapper;{{ end }}{{ end }}
{{ end -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.

Je me demande si à l'avenir on ne pourrait pas avoir d'autres messages qui n'étendraient pas DistributionElement.
Pour rappel, la propriété/balise "message" contient systématiquement un message fonctionnel, qui est étendu quasi systématiquement par les champs du DistributionElement => d'où la mécanique d'héritage, avec un UseCaseWrapper qui étend DistributionElement en y ajoutant le UseCase (CreateCaseWrapper = DistributionElement + CreateCase, EmsiWrapper = DistributionElement + Emsi, etc.).

On se pose quand même la question de ne pas porter systématiquement les champs du DistributionElement, qui est assez lourd et redondant avec des infos déjà présentes au niveau de l'enveloppe, pour les messages qui n'ont pas vocation à être interforces.

Si on va dans cette direction, on aura peut-être intérêt à terme à remplacer la mécanique "ne rootElement" par un attribut custom qu'on pourrait avoir également dans le tableur Excel.

Mais ça me paraît OK pour le moment !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines 118 to 122

def is_allowing_additional_properties():
return is_custom_content() or MODEL_TYPE == "DistributionElement"
def is_allowing_additional_properties(name):
return is_custom_content() or MODEL_TYPE == "DistributionElement" or name == "RC-DE"

Path('out/' + name).mkdir(parents=True, exist_ok=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

on utilise les deux, ou c'est qu'on a abandonné l'utilisation deMODEL_TYPE pour ne plus utiliser que "name" ?

Copy link
Author

@issam71100 issam71100 Jan 14, 2025

Choose a reason for hiding this comment

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

J'avais vu avec Saveliy qu'il fallait accepter les données supplémentaires spécialement pour le RC-DE, c'est pourquoi j'ai ajouté une condition sur le name. Quand au MODEL_TYPE je n'y ai pas spécialement touché.

Comment on lines +1 to +12
#!/bin/bash

# This script is used to generate the Java classes from the JSON Schemas and run the tests.
# It is intended to be run in a CI/CD environment, such as GitHub Actions.

set -e

echo "Cleaning up old generated schemas..."
# Clean up old generated schemas
find ./src/main/resources/json-schema -type f -name '*.json' ! -name 'customContent.schema.json' ! -name 'EDXL-DE-*.schema.json' -exec rm {} +

echo "Cleaning up old generated Java classes..."
Copy link
Collaborator

Choose a reason for hiding this comment

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

C'est ta version locale de la github action ? Je serais plutôt aprtisan de ne pas la versionner, car on risque de ne pas la maintenir iso avec la gh action au fil du temps, et donc de garder dans le repo un fichier dont on ne sera plus trop sûr...

def is_allowing_additional_properties():
return is_custom_content() or MODEL_TYPE == "DistributionElement"
def is_allowing_additional_properties(name):
return is_custom_content() or MODEL_TYPE == "DistributionElement" or name == "RC-DE"

Choose a reason for hiding this comment

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

par curiosité, pourquoi ajouter la condition sur le name ="RC-DE" ?

Copy link
Author

Choose a reason for hiding this comment

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

@bou3108 bou3108 merged commit 50e041d into auto/model_tracker Jan 15, 2025
@bou3108 bou3108 deleted the feature/parser/remove-a1-cell branch January 15, 2025 13:12
@issam71100 issam71100 added Review Ready The branch is ready for review (in a working state and/or review required) and removed WIP Work In Progress: the branch is still under active development and not stable / in a working state labels Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Ready The branch is ready for review (in a working state and/or review required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants