-
Notifications
You must be signed in to change notification settings - Fork 101
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
Améliore les mesures économiques #1207
Conversation
Merci @bfabre01 ! et @claireleroy à la review tenons bien compte du fait qu'il s'agit ici non pas d'un changement mineur mais bien d'un "breaking change" (et non des moindres, on a beaucoup de réutilisations qui utilisent directement Je prendrai le temps d'une review un peu plus approfondie dans les jours à venir et toute précision sur les tenants et aboutissants d'ici là sera la bienvenue… |
@Morendil : merci pour ton retour. Je pense qu'ajouter le suffixe |
c414892
to
9453026
Compare
Comme @Morendil , je suis assez circonspect sur l'ajout du suffixe
Dans l'ensemble, je crois qu'il y a plus de désagréments que de bénéfices à effectuer ce changement 🙂 |
@Morendil @fpagnoux : merci pour vos retours. On avait fait ce renommage sous taxipp, car on décline de notre côté certaines variables (comme |
b77d274
to
f728eb9
Compare
@bfabre01 Du coup comme changements effectués :
|
Il me semble qu'on a encore un breaking en l'état (b8c2f44) car on supprime:
|
@Morendil Yes
|
@claireleroy Oui il n'y a pas de souci pour supprimer des variables qui semblent superflues, on peut se faire confiance là dessus, ou pour des renommages en général (sauf certains cas particuliers sur lesquels on peut avoir des avis divergents), et comme on ne peut pas savoir quelles variables sont exploitées par les applis ou modules qui dépendent d'OpenFisca, on s'oblige à respecter le contrat Semver et déclarer un breaking, même s'il nous semble peu probable qu'il y ait vraiment des effets délétères. Comme ça tous ceux dont le |
En tout cas on ne le trouve plus sur la branche ! A vérifier… |
@Morendil Ok merci pour les précisions, j'ai updaté le numéro de version en ce sens ! |
|
@claireleroy @Morendil : concernant |
|
@claireleroy : ok ça marche, partons du fait que |
@bfabre01 @claireleroy Je reprends la main pour rebaser la PR afin d'éliminer l'erreur PyTest 4. Pensez à reprendre cette version de la branche si vous avez à ajouter des commits |
5cb3eee
to
8f164ac
Compare
@bfabre01 @claireleroy J'ai supprimé les modifications sur Changelog et setup.py: ça simplifie le rebase, et les rebase futurs - il est possible que cette PR traîne encore un peu et d'autres PR peuvent passer devant (edit #1199 est passée devant). Je les réintègrerai juste avant de merger. |
8f164ac
to
740bd5a
Compare
@claireleroy @bfabre01 Je vous laisse merger ou vous rends la main s'il y avait encore des modifications très mineures. (Si c'est plus substantiel, ouvrir une nouvelle PR serait plus indiqué.) |
mesures.py
prelevements_obligatoires/prelevements_sociaux/contributions_sociales/csg_crds.py
minima_sociaux
(pour prendre en compte la CRDS sur ces revenus, comme c'est le cas pouraides_logement
etprestations_familiales
.csg
pour inclure la CSG sur les revenus non salariésrevenus_nets_du_travail
plus_values_revenus_nets_du_capital
enplus_values_base_large
niveau_de_vie_net
revenu_net
revenu_net_individu