feat: enable schema extensions via data package architecture#7
Conversation
…nstants into class headers
|
/!\ ne pas merge cette PR Salut @Pierlou , La PR est prête à être review. Tests réalisés :
|
Pierlou
left a comment
There was a problem hiding this comment.
Thanks a lot for this huge refactor! Most comments are syntax/wording suggestions. Also not sure about the workflow around the datapackage file
Also for fields names, descriptions and typing, it'd be nice to have a review from people who actually know the topic 🙏
|
Merci pour la review. Ta précédente review était en francais, c'est une volonté d'etalab de passer en anglais ? Comme indiqué dans le 2nd message, les champs schémas ne sont pas à review car illustratifs pour montrer le fonctionnement de la partie technique. Il seront supprimés avant de merge. Je me suis donc permis de clore les messages liés. Merci pour les suggestions sur la quasi totalité des autres commentaires que j'ajouterai demain à la PR. Le seul point restant serait celui-çi :
Effectivement, actuellement, le repo ne génère pas un datapackge mais une collection de table-schema.json => https://specs.frictionlessdata.io/schemas/table-schema.json indépendants. Quand je vois le fichier que tu as link, je me rend compte que je n'avais pas du tout en tête la bonne définition d'un datapackage et c'est ce qui explique que j'avais abandonné cette solution. |
|
Désolé pour la review en anglais, on a l'habitude de faire ça dans le pôle data (même si ce dont on parle est franco-français) je ne me suis pas posé la question 😅
Ce repo étant voué à accueillir plusieurs TableSchemas, c'est le second point qui me semble adapté. La structure attendue est dans le lien que j'ai donné, pour un rendu similaire à ceci (un sous-menu qui expose les différents schémas, ce qui évite de surcharger la page d'accueil avec beaucoup de schémas liés à un seul sujet). Les modifs pour arriver à cela me semblent assez légères, le très gros du travail a déjà été fait 💪 |
|
I can switch to English, that’s not a problem. Thanks a lot for your review and for the link to the datapackage.json example — it’s much clearer to me now what was expected. Everything should be back in a reviewable state. I expect this PR to be closer to the final result, but please feel free to nitpick if you see further improvements. Out of curiosity, what did you mean by “d'autres structures pour des données non tabulaires donc non pertinentes pour le cas présent”? When we chose table-schema a year ago, one of the main arguments was that it was best supported by schema.data.gouv and publier.etalab. I’d be interested to know what other options were available then, or what you would recommend now. |
|
I'll try to review the changes next week, thanks for the quick fixes/improvements!
datagouv is very muh turned towards tabular data, we have a bunch of automatic processes around them (column content detection, APIfication, conversions into other formats...) so a TableSchema for this use case felt like the best option |
Pierlou
left a comment
There was a problem hiding this comment.
Looks good to me! 👏 Again, I'd be happy to have someone else review this, ideally someone who'll be using it
Once this is done we can merge, create a release, and see what it looks like on our preprod platform
|
Bon ça y est @ttdm j'ai enfin eu du temps pour me plonger dans cette PR en étant pleinement concentré sur le sujet. Ce que je peux en dire :
Merci pour tout ce travail, on en parle demain. Sans lien direct avec cette PR mais en lien avec ce schéma, en ce qui me concerne après une première implémentation d'une API en écriture basée sur le schéma cœur, j'ai un gros grief sur notre choix, dont je doutais déjà à l'époque, du champ |
|
@David-Guillot
Sur le format choisi, de mon point de vue la question n'était pas sur minimiser le nombre de champ mais sur un format tabulaire vs non-tabulaire, la PR a été l'occasion d'en rediscuter rapidement avec Pierlou et je renvoie vers ce message : #7 (comment) Sur les tests, je suis tout à fait d'accord. C'est une de mes faiblesses en tant que dev en général. J'ai très souvent l'impression que les tests que j'écris sont trop triviaux pour être intéressants. Tu aurais le temps de proposer quelque chose ? |
|
Pour le format tabulaire : ce n'est pas obligatoire, datagouv met mieux en valeur ce format que les autres (prévisualisation, APIfication, conversion dans d'autres formats...), mais il reste tout à fait possible de publier des données dans d'autres formats, notamment le JSON. Si vous voulez creuser dans cette direction, il est possible de formaliser un JSONSchema |
|
@ttdm OK j'ai compris que les commentaire de Pierlou sur lesquels j'avais focalisé concernent des fichiers qui n'avaient pas vocation à être commités. En fait, ce que je vais faire, c'est déplacer des choses dans un répertoire de tests, et réutiliser les schémas sources et build que tu qualifies d'illustratifs et temporaires, et en faire un jeu de tests (source => fixture, et build => expected). |
… and exemple files.
|
Ok @ttdm merci pour la review de ma PR, tu es même allé plus loin que ce que je pensais vu que tu as résolu les inévitables problèmes liés à un workflow Github qu'on n'a pas pu tester avant 👍 (je pensais qu'on allait itérer ensemble sur ce sujet après que le premier run soit passé, merci d'avoir pris ça en charge). J'ai une question sur ton dernier commit, mais elle n'est pas bloquante pour la validation finale : je suis étonné de voir que le contenu de En dehors de cette question, pour moi en l'état on est OK pour envoyer tout ça 👍 |
Très franchement j'avais un peu honte de la source de l'erreur. Je pense que c'est du code qui ne va pas beaucoup bouger et rester relativement simple, d'où les libertés que j'ai pris sur un certain nombre de bonne pratique. Mais quand j'ai vu que l'erreur venait d'une path codée en statique à 2 endroits différents, je me suis dis que ça valait le coup d'améliorer un peu cela !
Les deux sont possibles, commit manuels et commit via le workflow. La seule différence c'est de lancer le script de build en local ou via le workflow. Comme j'étais sur le code et que j'étais en train de fix le script de build, je l'ai fait en local pour vérifier que ma proposition était bien fonctionnelle. (Ton erreur au build que j'ai fix éxistait aussi lorsque que tu lancais le script en local par exemple.) @Pierlou On est donc tout bon, tu peux merge ! |
/!\ ne pas merge cette PR les données utilisées pour générer les schémas sont des illustratives permettant de vérifier le bon fonctionnement du code proposé
Génération automatique des schémas par combinaison core + extensions
Cette PR introduit une architecture modulaire pour la génération des schémas du dispositif d'aide.
Principe
Le schéma est désormais découpé en :
Un script de build (src/build_schemas.py) génère automatiquement toutes les combinaisons core × cible × usage et produit les schémas finaux dans build/schemas/.
Ce que ça change concrètement
Structure des sources
schema/ => Contient uniquement des fichiers json permettant de créer des schéma valides en se combinant les uns les autres
Build/ => Dossier autogénéré comprennant toutes les combinaisons de json schema valides
src/ => Contient quelques fichiers sources permettant 1/de générer les schéma 2/ de gérer les conflits 3/ de valider les schémas générés
Note : tout ce qui se trouve dans build/ est auto-généré — ce dossier n'est pas à review.