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 versement transport rates #726

Merged
merged 4 commits into from
Apr 20, 2017
Merged

Update versement transport rates #726

merged 4 commits into from
Apr 20, 2017

Conversation

laem
Copy link
Contributor

@laem laem commented Apr 5, 2017

  • Évolution du système socio-fiscal, Amélioration technique
  • Périodes concernées : de 2016 à 2017
  • Zones impactées : openfisca_france/assets/versement_transport/.
  • Détails :
    • Mise à jour des taux de versement transport suite aux changements annuels du mois d'avril
    • Nettoyage des anciens fichiers, pour réduire la taille du dépôt à partir de ce commit

Ces changements :

  • Corrigent ou améliorent un calcul déjà existant.
  • Modifient des éléments non fonctionnels de ce dépôt (nettoyage des fichiers d'assets).

@laem laem self-assigned this Apr 5, 2017
@laem laem force-pushed the maj-taux-versement-transport branch from 2b23104 to 5cd6da7 Compare April 12, 2017 16:13
@laem laem changed the title [WIP] Update versement transport rates Update versement transport rates Apr 13, 2017
@laem laem requested a review from benjello April 13, 2017 10:47
@laem
Copy link
Contributor Author

laem commented Apr 13, 2017

@fpagnoux After this merge you might want to add the option --depth to the 'install for developpers' section of the doc if ~8 mega less to download makes a difference

@fpagnoux
Copy link
Member

Thanks for the advice !
image

The competition to become the best code remover is getting fierce 😆

@laem
Copy link
Contributor Author

laem commented Apr 18, 2017

I will merge this tomorrow, unless something's wrong @benjello

@benjello
Copy link
Member

benjello commented Apr 19, 2017

@laem : pourrais-tu préciser quels sont les fichiers qui ont été virés stp ? Et lequel reste.
L'idée est de récupérer au moins le fichier d'origine.
Si tu as un autre source pour ces taux je suis également preneur.
Je rapatrierai tout cela ailleurs.
Merci d'avance.

@laem
Copy link
Contributor Author

laem commented Apr 19, 2017

Le seul qui reste, taux.json, est un fichier des taux historisés, à jour au 1er avril 2017, par commune, AOT et SMT, récupéré sur URRSAF.fr.
J'avais dans le passé ajouté l'équivalent en .yaml, pour la lisibilité. Mais il s'avère que YAML est bien trop lent à parcourir (en python au moins), donc je l'enlève.
Les .csv sont les fichiers historiques d'OpenFisca je crois, qui ne contiennent pas l'historique et sont périmés. Le fichier extract_taux était utilisé pour les fichiers csv.

@laem
Copy link
Contributor Author

laem commented Apr 19, 2017

Exemple (github ne gère pas ces diffs...) :

Par exemple, Brest dans le nouveau .json :

'29019':
  codePostal: '29200'
  nomLaposte: BREST
  nomAcoss: BREST
  aot:
    nom: COMMUNAUTE URBAINE DE BREST
    taux:
      '2006-01-01': '1.65'
      '1993-03-01': '1.05'
      '1992-06-01': '1.1'
      '1975-07-01': '1'
      '2009-09-01': '1.8'

Dans l'ancien .csv :

29200;29019;BREST ;BREST;COMMUNAUTE URBAINE DE BREST;1,8;;

@laem laem force-pushed the maj-taux-versement-transport branch 2 times, most recently from b92407e to 1194749 Compare April 20, 2017 08:36
@laem
Copy link
Contributor Author

laem commented Apr 20, 2017

@benjello @fpagnoux @cbenz pouvez-vous merger cette PR svp ? Ou m'ajouter à nouveau aux contributeurs -france ?

selection_388

@benjello
Copy link
Member

@fpagnoux : tout est ok pour moi
@laem : je laisse @fpagnoux merger ou te donner les droits

@laem
Copy link
Contributor Author

laem commented Apr 20, 2017

@fpagnoux

Pour pouvoir faire tourner les tests d'une PR, il faut que setup.py et CHANGELOG.md soient à jour, donc que l'on y ait défini et inscrit une nouvelle version.

Sauf que si quelqu'un pousse entre temps, c'est obsolète, il faut rebaser et résoudre les conflits, 3 fois pour cette PR (ou alors je m'y prends mal ?). Ce problème devrait s'empirer avec le nombre de contributeurs.

J'imagine que ce n'est pas simple, mais ce serait super de pouvoir définir le type de mise à jour (major, minor, patch) et la description du CHANGELOG dans le texte de la PR, puis que le CI se charge d'incrémenter la version et d'inscrire ces infos.

@laem laem force-pushed the maj-taux-versement-transport branch from 1194749 to 5a262d8 Compare April 20, 2017 10:30
@laem laem merged commit cc9be1b into master Apr 20, 2017
@laem laem deleted the maj-taux-versement-transport branch April 20, 2017 10:59
@fpagnoux
Copy link
Member

fpagnoux commented Apr 26, 2017

@laem

En général, j'adopte le workflow suivant :

  • J'ouvre ma PR
  • Je demande une review.
    • À partir de ce moment je m'interdis de rebaser, vu que les force-push sont pénibles pour les reviewers.
  • Je fais évoluer mon code en fonction des remarques
    • Si par hasard les tests de la PR ne passent plus parce que quelqu'un a publié un package avec la même version que moi, je me base sur les tests 'push' de github, qui sont strictement équivalents d'un point de vue de la non-régression.
  • Une fois ma PR est approuvée, je rebase et je résous les conflicts potentiels sur les setup.py et le CHANGELOG.md. Ces conflits sont très fréquents, mais leur résolution est triviale.
  • Dès que les tests sont passés, je merge.

En général, je ne résous les conflits qu'une seule fois.

J'imagine que ce n'est pas simple, mais ce serait super de pouvoir définir le type de mise à jour (major, minor, patch) et la description du CHANGELOG dans le texte de la PR, puis que le CI se charge d'incrémenter la version et d'inscrire ces infos.

Fonctionnellement ce serait super, mais je n'ai aucune idée de comment faire. Il faudrait que travis est connaissance du texte de la PR, ce qui est peut-être possible (il connaît le numéro de la PR par exemple). Si tu as le temps et que tu veux creuser ça, tu es le bienvenue :).

@MattiSG
Copy link
Member

MattiSG commented Apr 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants