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

Rendre OpenFisca France clonable sous Windows #825

Merged
merged 12 commits into from
Sep 25, 2017
Merged

Conversation

sandcha
Copy link
Contributor

@sandcha sandcha commented Sep 22, 2017

Fixes #819

  • Correction d'un crash sous Windows
  • Zones impactées : openfisca_france/parameters.
  • Détails :
    • Dans la hiérarchie de fichiers de paramètres, assemble dans index.yaml le contenu des sous-répertoires lorsque ceux-ci contiennent un fichier de chemin trop long (plus de 150 caractères à partir d' openfisca-france/openfisca_france/
    • Corrige l'erreur WindowsError: [Error 206] Nom de fichier ou extension trop long lors du clone d'OpenFisca-France sous Windows

Copy link
Member

@fpagnoux fpagnoux left a comment

Choose a reason for hiding this comment

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

Le comportement du script me paraît étrange.

Par exemple, prestations_familiales.af.af_dom.duree_pendant_laquelle_la_majoration_pour_les_plus_de_20_ans_est_versee_en_annee est trop long.

Pour le raccourcir, il suffirait de remplacer le répertoire prestations_familiales/af/af_dom/ par un fichier prestations_familiales/af/af_dom.yaml, qui contiendrait les sous noeuds.

Ici, le script déplace tout le sous-noeud af_dom dans le index.yaml de prestations_familiales/af/.

Ce n'est pas nécessaire, et il se trouve... que ça ne fonctionne pas !

Quand les index.yaml on été rajoutés, ils ont été pensé uniquement comme moyen de permettre de rajouter des méta-donnée sur un noeud de la législation. Les sous-noeuds déclarés dedans ne sont pas parsés. Pour déclarer un noeud:

  • Soit on écrit un fichier af_dom.yaml qui déclare des sous-noeuds
  • Soit on créé un répertoire af_domqui contient d'autres yaml, et éventuellement un index.yaml pour les metadonnées

Je peux comprendre que ce comportement soit surprenant, mais faire des tests basiques à la main avant de scripter permet de voir venir ce genre de problèmes...

@@ -1,2 +1,156 @@
description: Allocations familiales
reference: ipp
af_dom:
age_1er_enf_tranche_1_dom:
description: "\xC2ge de d\xE9but de la 1ere majoration"
Copy link
Member

Choose a reason for hiding this comment

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

Problème d'encodage.

Copy link
Member

Choose a reason for hiding this comment

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

Je ne le signale qu'une fois, mais il y a en beaucoup d'autres.

2004-01-01:
value: 6.0
ass_mat1:
description: "Taux pour recours \xE0 une assistante maternelle, une association,\
Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi ce \ en de ligne ? Windows limite-t-il aussi le nombre de caractères par ligne :trollface: ?
Tout laisser sur une ligne, les éditeurs de texte modernes savent gérer le wrapping.

Copy link
Contributor Author

@sandcha sandcha Sep 22, 2017

Choose a reason for hiding this comment

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

Les fichiers source contenaient des descriptions sur plusieurs lignes (exemple avec prestations/prestations_familiales/paje/clmg/ass_mat1.yaml).
Ok pour tout mettre sur une ligne !

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Si c'était déjà comme ça avant, pas d'obligation, mais en tout cas il ne faut pas rajouter les \.

for f in files:
file_path = os.path.join(directory, f)
if (len(file_path) - PATH_LENGTH_TO_IGNORE) > PATH_MAX_LENGTH:
# print str((len(file_path) - PATH_LENGTH_TO_IGNORE)) + " " + file_path
Copy link
Member

Choose a reason for hiding this comment

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

Remaining commented line



long_parameters_paths = list_long_paths(PARAMETERS_DIRECTORY)
print str(len(long_parameters_paths)) + " directories have files with more than " + str(PATH_MAX_LENGTH) + \
Copy link
Member

Choose a reason for hiding this comment

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

To include variable values in a string in Python, use format:

print "{} directories have files with more thant {} characters in...".format(long_parameters_paths, PATH_MAX_LENGTH, PARENT_DIRECTORY).encode('utf-8')

@sandcha
Copy link
Contributor Author

sandcha commented Sep 22, 2017

Dans impot_revenu/plus_values.yaml, il y a de la reference ipp et openfisca. Faudrait-il supprimer la reference: ipp englobante ? (ligne 2 donc)

@sandcha
Copy link
Contributor Author

sandcha commented Sep 22, 2017

Dans l'ensemble, les references qui sont à la racine des nouveaux yaml sont redondantes avec le reste du contenu. Est-ce qu'on les supprime en se basant sur l'hypothèse qu'avant notre intervention, tout fichier yaml contenait déjà une référence ?

@sandcha sandcha requested a review from fpagnoux September 22, 2017 19:08
Copy link
Member

@fpagnoux fpagnoux left a comment

Choose a reason for hiding this comment

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

Thanks for taking this tedious task 🙂 .

I checked on Windows that git clone -b params-paths-819 https://github.com/openfisca/openfisca-france.git was working. I hope we won't have trouble with the git history...

@fpagnoux fpagnoux merged commit c318679 into master Sep 25, 2017
@fpagnoux fpagnoux deleted the params-paths-819 branch September 25, 2017 08:38
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.

2 participants