Conserver les paramètres modifiés à l'import des barèmes IPP #778

michelbl · 2017-06-12T17:51:11Z

Connected to #752

michelbl · 2017-06-13T12:46:19Z

Je n'ai pas fait le merge avec les paramètres de l'IPP parce que ce n'est pas le sujet de l'issue, et que ça laisse l'occasion aux reviewers de constater à quoi ressemble le diff dans des conditions réelles.

Un modification récente (12/06/2017) des XLSX de l'IPP a cassé le script de transformation. @benjello est au courant, cf la discussion sur le channel #of-france et https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-xlsx/issues/93. J'ai ajouté la possibilité de choisir la version des XLSX à importer, et j'ai testé sur une version antérieure.

MattiSG

Impossible à tester de mon côté car j'ai l'erreur connue sous macOS.

MattiSG · 2017-06-13T14:15:49Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

-```sh
-./openfisca_france/scripts/parameters/baremes_ipp/convert_ipp_xlsx_to_openfisca_xml.py --xml-dir openfisca_france/parameters
-```
+Plus précisément, il réécrit les fichiers de paramètres d'OpenFisca-France en conservant leur structure, tout en remplaçant les valeurs par celles provenant de l'IPP. Le contributeur voit apparaître un *diff* dans *git*, et peut choisit manuellement d'ajouter ou non les lignes modifiées dans un *commit*.


choisit → choisir

MattiSG · 2017-06-13T14:18:00Z

openfisca_france/scripts/parameters/baremes_ipp/convert_ipp_xlsx_to_openfisca_xml.py

@@ -29,7 +29,8 @@


 app_name = os.path.splitext(os.path.basename(__file__))[0]
-default_zip_url = u'https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-xlsx/repository/archive.zip?ref=master'  # noqa
+ref = '2c6936d6'


Le fait de fixer la référence plutôt que d'utiliser master signifie que ce script ne mettra pas à jour les données à la dernière version. Il faut a minima prendre cette référence en paramètre et le documenter, et probablement également la mettre comme valeur par défaut à master (quitte à indiquer dans le README que la dernière version compatible connue est 2c6936d6 et que master ne fonctionne actuellement pas).

MattiSG · 2017-06-13T14:30:28Z

J'ai créé #779 suite à mes tests.

fpagnoux

Pour que cet script soit vraiment utilisé, il faut à mon avis un script à copier-coller qui fait tout le boulot de A à Z. Pour le moment, on a encore deux étapes.

fpagnoux · 2017-06-14T09:05:21Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


 ```sh
-pip install xlrd
-# Ou bien utilisez l'extra-require comme ceci : pip install --editable ".[baremes_ipp]"
+pip install .[baremes_ipp]


Attention, sans le -e, on n'installe pas en editable. pip va donc copier ces sources vers ses dossiers d'installation, sans garder de lien.

Or le script de merge est fait pour écrire dans le dossier où est installé OpenFisca France. Si on le fait tourner, les paramètres sont mis à jours dans les dossiers secrets de pip, et il n'y a donc pas de diff.

fpagnoux · 2017-06-14T09:10:54Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

@@ -45,15 +44,13 @@ INFO:convert_ipp_xlsx_to_openfisca_xml:XML files written to '/tmp/baremes-ipp-v3

 > Ce processus de conversion de données a été internalisé dans OpenFisca-France à partir de celui de l'IPP disponible [ici](https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-converters#in-the-ipp-world).

-### Intégration avec OpenFisca
+> La dernière version du repository `ipp-tax-and-benefit-tables-xlsx` testée est `2c6936d6`. Utiliser l'argument `--ref-ipp` pour spécifier une référence à utiliser autre que `master`.


Ok, mais il faut s'attendre à ce que toute personne ingénue qui arrive sur ce README (dont moi il y a 30 minutes):

copie colle la première ligne (./openfisca_france/.../convert_ipp_xlsx_to_openfisca_xml.py)

constate que ça ne fonctionne pas en recevant une erreur incompréhensible (TypeError: unhashable type: 'OrderedDict')

Parte en râlant que de-toute-façon-ça-marche-pas

Je préfère qu'on est une ligne copie-collable, avec un commentaire à côté qui explique pourquoi on a une option supplémentaire.

fpagnoux · 2017-06-14T09:12:40Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


-Pour cela, utiliser l'option `--xml-dir` du script :
+Le script [`merge_ipp_xml_files_with_openfisca_parameters.py`](./merge_ipp_xml_files_with_openfisca_parameters.py) fusionne les fichiers XML produits depuis les fichiers XLSX de l'IPP avec les fichiers XML existants d'OpenFisca-France.


En tant qu'utilisateur, je suis plus interressé par une commande copie-collable plus que par un lien vers la source du script.

Par ailleurs, il faudrait documenter les paramètres attendus de ce script (en l'occurence, le dossier temporaire qui contient les xml).

fpagnoux · 2017-06-14T22:09:06Z

I unified the scripts, and documented a way to use the server for those of us who use macOS.
Reviews welcome:

@MattiSG as you already spent some time on the issue
@sandcha or @Anna-Livia to have a fresh look at the instructions and see if everything is clear.

You will need access to the openfisca france server. If you don't have access yet, let me know, and we'll fix that 🙂 .

sandcha

Instructions globalement claires. Cela n'a pas généré d'erreur sur serveur et j'ai obtenu le même résultat en local, sous macOS. 😶

sandcha · 2017-06-19T14:12:21Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

@@ -10,51 +10,88 @@ L'IPP produit des fichiers au format XLSX appelés « Barèmes IPP » :

 Ces barèmes IPP sont très complets et il est intéressant pour OpenFisca de profiter de leur contenu.

-## Conversion XLSX vers XML
+Les scripts décrits ici permettent d'importer les barèmes IPP (fournis sous forme de tableurs au format XLSX) dans [les fichiers de paramètres](https://doc.openfisca.fr/coding-the-legislation/legislation_parameters.html) XML d'OpenFisca France.


Distinguer le lien vers la doc et le lien vers les fichiers xml auxquels on pourrait s'attendre avec ce tag ?

sandcha · 2017-06-19T15:06:36Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


 ```sh
-pip install xlrd
-# Ou bien utilisez l'extra-require comme ceci : pip install --editable ".[baremes_ipp]"
+pip install --editable .[baremes_ipp]
 ```

 Il faut également installer `gnumeric` :


Indiquer une version de gnumeric ?

sandcha · 2017-06-19T15:31:20Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

 ```

-Le script génère un nouveau répertoire dans `/tmp` à chaque exécution, dont une partie du nom est aléatoire. Ce répertoire n'est pas effacé par le script. Par exemple :
+> L'argument `--ref-ipp` permet de spécifier une version spécifique des [fichiers `XLSX` publiés par l'IPP](https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-xlsx/repository/archive.zip) à utiliser.


Préciser qu'il s'agit de version git ? En particulier parce qu'on désigne des fichiers Excel.
Et, dans cette phrase générique, mettre plutôt un lien vers le dépôt IPP ?

sandcha · 2017-06-19T15:38:10Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

+./openfisca_france/scripts/parameters/baremes_ipp/convert_ipp_xlsx_to_openfisca_xml.py --ref-ipp 2c6936d6 --merge
+git commit --all -m "Update baremes IPP"
+git push origin $BRANCH_NAME
+git checkout baremes-ipp-merge2


sandcha · 2017-06-19T15:53:52Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


-OpenFisca définit ses paramètres dans un format XML décrit [ici](https://doc.openfisca.fr/coding-the-legislation/legislation_parameters.html).
+## Imports des barèmes IPP dans OpenFisca



Ajouter une phrase d'introduction désignant le nom du script de conversion ?

En tant qu'utilisateur du scrit, je ne pense pas que le nom me soit très utile. Je veux juste pouvoir effectuer l'import.

Mais j'ai changé "Ce script" en "L'import nécessite" pour qu'on oublie encore plus le script 🙂

Anna-Livia · 2017-06-20T08:25:35Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


 ```sh
-pip install xlrd
-# Ou bien utilisez l'extra-require comme ceci : pip install --editable ".[baremes_ipp]"
+pip install --editable .[baremes_ipp]


Il faudrait mettre des guillements autour de
.[baremes_ipp]

Au cas où l'utilisateur ait un terminal type zsh qui donne un sens aux éléments entre crochets.

Anna-Livia · 2017-06-20T08:31:09Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

 ```

 Il faut également installer `gnumeric` :
 - sous Debian : `sudo apt install gnumeric`
- sous macOS : `brew install gnumeric`
+
+#### Utilisation

 Se placer dans le répertoire racine d'OpenFisca-France, là où se trouve le fichier `setup.py` :


Je pense qu'ajouter "et executer : " ou donner une instruction peut clarifier la procédure.

Anna-Livia · 2017-06-20T08:34:16Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

+> L'argument `--ref-ipp` permet de spécifier une version spécifique des [fichiers `XLSX` publiés par l'IPP](https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-xlsx/repository/archive.zip) à utiliser.
+> La dernière version de ces fichiers compatible avec le script d'import est `2c6936d6`. Pour utiliser les fichiers les plus récents, retirer le paramètre `--ref-ipp`.
+
+Une fois le script exécuté, une branche `update-baremes-ipp-YYYY-MM-DD-HH-MM` (par exemple `update-baremes-ipp-2017-06-14-15-30` si le script est exécuté le 14 juin 2017 à 15h30) est publiée sur ce dépôt. Le contributeur peut alors éditer le code sur cette branche, et ouvrir une pull request.


Je propose :

Une fois le script exécuté, une branche update-baremes-ipp-YYYY-MM-DD-HH-MM est publiée sur ce dépôt.

Exemple : update-baremes-ipp-2017-06-14-15-30 si le script est exécuté le 14 juin 2017 à 15h30

Le contributeur peut alors éditer le code sur cette branche, et ouvrir une pull request.

Anna-Livia · 2017-06-20T08:40:13Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


 ```sh
-./openfisca_france/scripts/parameters/baremes_ipp/convert_ipp_xlsx_to_openfisca_xml.py --xml-dir openfisca_france/parameters
+./openfisca_france/scripts/parameters/baremes_ipp/merge_ipp_xml_files_with_openfisca_parameters.py /tmp/baremes-ipp-v3SAEz/xml # Par exemple. Remplacer par le dossier temporaire mentionné précedemment qui contient les XML.


Je propose de mettre "par exemple" avant, pour qu'on comprenne le lien avec le paragraphe au dessus.

Serait-il possible de reformuler l'exemple, j'ai du mal à le comprendre :/

Anna-Livia · 2017-06-20T08:40:41Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

 ```

+Plus précisément, il réécrit les fichiers de paramètres d'OpenFisca-France en conservant leur structure, tout en remplaçant les valeurs par celles provenant de l'IPP. 


Anna-Livia · 2017-06-20T08:45:24Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

+
+Le script [`convert_ipp_xlsx_to_openfisca_xml.py`](./convert_ipp_xlsx_to_openfisca_xml.py) prend en charge la conversion de XLSX vers XML.
+
+Ce script génère un nouveau répertoire dans `/tmp` à chaque exécution, dont une partie du nom est aléatoire. Ce répertoire n'est pas effacé par le script. Par exemple :

 ```
 INFO:convert_ipp_xlsx_to_openfisca_xml:XML files written to '/tmp/baremes-ipp-v3SAEz/xml'


Je propose de rendre l'objectif de l'exemple plus explicite :

donner un exemple de nom aléatoire ?

repérer dans le processus le nom généré pour facilement le retrouver.

Est-ce que je peux supprimer le ficher temporaire où faut-il que je le garde ?

Je n'aurai pas le temps de traiter une nouvelle revue, et d'autres personnes en ont fait.

Anna-Livia · 2017-06-21T18:51:26Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


 ```sh
-./openfisca_france/scripts/parameters/baremes_ipp/convert_ipp_xlsx_to_openfisca_xml.py
+ssh baremes-ipp@openfisca.fr -A


J'ai essayé de lancer la commande et je n'ai pas les accès --> Je propose d'indiquer aux lecteurs que cette partie n'est accessible qu'aux personnes ayant les accès

Anna-Livia · 2017-06-21T18:54:52Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

+> L'argument `--ref-ipp` permet de spécifier une version spécifique des [fichiers `XLSX` publiés par l'IPP](https://framagit.org/french-tax-and-benefit-tables/ipp-tax-and-benefit-tables-xlsx/repository/archive.zip) à utiliser.
+> La dernière version de ces fichiers compatible avec le script d'import est `2c6936d6`. Pour utiliser les fichiers les plus récents, retirer le paramètre `--ref-ipp`.
+
+Une fois le script exécuté, une branche `update-baremes-ipp-YYYY-MM-DD-HH-MM` (par exemple `update-baremes-ipp-2017-06-14-15-30` si le script est exécuté le 14 juin 2017 à 15h30) est publiée sur ce dépôt. Le contributeur peut alors éditer le code sur cette branche, et ouvrir une pull request.


Je propose :

Une fois le script exécuté, une branche update-baremes-ipp-YYYY-MM-DD-HH-MM est publiée sur ce dépôt.

Exemple : update-baremes-ipp-2017-06-14-15-30 si le script est exécuté le 14 juin 2017 à 15h30

Le contributeur peut alors éditer le code sur cette branche, et ouvrir une pull request.

Anna-Livia · 2017-06-21T18:57:01Z

openfisca_france/scripts/parameters/baremes_ipp/README.md

+
+Le script [`convert_ipp_xlsx_to_openfisca_xml.py`](./convert_ipp_xlsx_to_openfisca_xml.py) prend en charge la conversion de XLSX vers XML.
+
+Ce script génère un nouveau répertoire dans `/tmp` à chaque exécution, dont une partie du nom est aléatoire. Ce répertoire n'est pas effacé par le script.


"Ce répertoire n'est pas effacé par le script." --> Quel doit être l'action de l'utilisateur face à cette info ? Est-ce que c'est un bug ou une feature ?

Anna-Livia · 2017-06-21T19:00:19Z

openfisca_france/scripts/parameters/baremes_ipp/README.md


+Par exemple, si le dossier temporaire qui contient les `XML` produits par le script de conversion précédent est `/tmp/baremes-ipp-v3SAEz/xml`, exécuter :


Je propose :

Exemple : si le dossier temporaire qui contient les XML produits par le script de conversion précédent est /tmp/baremes-ipp-v3SAEz/xml, exécuter :

Anna-Livia · 2017-06-21T19:13:18Z

...fisca_france/scripts/parameters/baremes_ipp/merge_ipp_xml_files_with_openfisca_parameters.py

+def merge_attributes(openfisca_element, ipp_element):
+    for key, value in ipp_element.attrib.iteritems():
+        if key in openfisca_element.attrib:
+            if openfisca_element.attrib[key] != ipp_element.attrib[key]:


Est ce que ce ne serait pas intéressant de décrire la situation où on ne remplace pas:
(key in openfisca_element.attrib) and (openfisca_element.attrib[key] == ipp_element.attrib[key])
et de dire qu'on remplace pour toutes les autres ?

Deuxième question : Pourquoi itérer sur key/value, si on n'utilise pas value ?

Tu as sans doute raison, mais je ne préfère pas prendre le risque de modifier le script, qui a été validé par plusieurs personnes, vu les contraintes de temps qui sont les nôtres.

Simplify install

Retours pris en compte

michelbl changed the title ~~Baremes ipp merge2~~ Conserver les paramètres modifiés à l'import des barèmes IPP Jun 12, 2017

michelbl self-assigned this Jun 12, 2017

michelbl requested a review from fpagnoux June 13, 2017 12:16

michelbl requested a review from MattiSG June 13, 2017 13:37

MattiSG previously requested changes Jun 13, 2017

View reviewed changes

fpagnoux requested changes Jun 14, 2017

View reviewed changes

fpagnoux force-pushed the baremes-ipp-merge2 branch from 87a5b22 to 8cbeb19 Compare June 14, 2017 22:04

fpagnoux approved these changes Jun 14, 2017

View reviewed changes

fpagnoux requested review from MattiSG, Anna-Livia and sandcha June 14, 2017 22:09

sandcha previously requested changes Jun 19, 2017

View reviewed changes

Anna-Livia reviewed Jun 20, 2017

View reviewed changes

fpagnoux requested review from Anna-Livia and sandcha June 21, 2017 17:45

Anna-Livia reviewed Jun 21, 2017

View reviewed changes

fpagnoux force-pushed the baremes-ipp-merge2 branch from b9e0e2d to 065b6b9 Compare June 21, 2017 23:58

Christophe Benz and others added 4 commits June 22, 2017 02:00

Add README

8116e7c

Permet de choisir la version XLSX

caf705f

Add merge script

3017eac

Ajout un script de formatage des XML

33130f0

Michel Blancard and others added 8 commits June 22, 2017 02:00

Reformate les XML

c699fab

Version + Changelog

a17c4d1

Add missing dependency to baremes IPP import

20c6175

Simplify install

Add a copy-pastable command to merge params

9d9f731

Unify convert and merge script

90653e3

Document server-side import

96af96c

Improve merge instructions

f804ae6

Improve IPP import instructions

5c3fe2b

fpagnoux force-pushed the baremes-ipp-merge2 branch from 065b6b9 to 5c3fe2b Compare June 22, 2017 00:00

fpagnoux merged commit 1da596e into master Jun 22, 2017

fpagnoux deleted the baremes-ipp-merge2 branch June 22, 2017 00:20


		Pour cela, utiliser l'option `--xml-dir` du script :
		Le script [`merge_ipp_xml_files_with_openfisca_parameters.py`](./merge_ipp_xml_files_with_openfisca_parameters.py) fusionne les fichiers XML produits depuis les fichiers XLSX de l'IPP avec les fichiers XML existants d'OpenFisca-France.


		OpenFisca définit ses paramètres dans un format XML décrit [ici](https://doc.openfisca.fr/coding-the-legislation/legislation_parameters.html).
		## Imports des barèmes IPP dans OpenFisca

		```

		Plus précisément, il réécrit les fichiers de paramètres d'OpenFisca-France en conservant leur structure, tout en remplaçant les valeurs par celles provenant de l'IPP.


		Le script [`convert_ipp_xlsx_to_openfisca_xml.py`](./convert_ipp_xlsx_to_openfisca_xml.py) prend en charge la conversion de XLSX vers XML.

		Ce script génère un nouveau répertoire dans `/tmp` à chaque exécution, dont une partie du nom est aléatoire. Ce répertoire n'est pas effacé par le script.


		Par exemple, si le dossier temporaire qui contient les `XML` produits par le script de conversion précédent est `/tmp/baremes-ipp-v3SAEz/xml`, exécuter :

Conserver les paramètres modifiés à l'import des barèmes IPP #778

Conserver les paramètres modifiés à l'import des barèmes IPP #778

Conversation

michelbl commented Jun 12, 2017

michelbl commented Jun 13, 2017 • edited Loading

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiSG commented Jun 13, 2017

fpagnoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented Jun 14, 2017 • edited Loading

sandcha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Anna-Livia Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anna-Livia Jun 21, 2017 • edited by fpagnoux Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anna-Livia Jun 21, 2017 • edited by fpagnoux Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anna-Livia Jun 21, 2017 • edited by fpagnoux Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelbl commented Jun 13, 2017 •

edited

Loading

fpagnoux commented Jun 14, 2017 •

edited

Loading

sandcha left a comment •

edited

Loading

fpagnoux Jun 21, 2017 •

edited

Loading

Anna-Livia Jun 20, 2017 •

edited

Loading

Anna-Livia Jun 21, 2017 •

edited by fpagnoux

Loading

Anna-Livia Jun 21, 2017 •

edited by fpagnoux

Loading

Anna-Livia Jun 21, 2017 •

edited by fpagnoux

Loading