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

Fix ppe_tp_sa #687

Merged
merged 2 commits into from
Mar 6, 2017
Merged

Fix ppe_tp_sa #687

merged 2 commits into from
Mar 6, 2017

Conversation

benjello
Copy link
Member

No description provided.

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.

Typo dans le changelog

CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
# Changelog

## 13.2.1

* Corrige la valeur erronnée retournée par `ppe_tp_sa`ppe_tp_sa
Copy link
Member

Choose a reason for hiding this comment

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

Typo dans le chanbelog

return period, individu('contrat_de_travail', period) > 0
mois = period.this_month
indicateur = individu('contrat_de_travail', mois) == 0
while mois.start.month < 12:
Copy link
Member

Choose a reason for hiding this comment

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

Un commentaire serait le bienvenue pour expliquer ce que ce fait ce code, l'emploi d'un while étant inhabituel sur des formules openfisca.

@benjello
Copy link
Member Author

@fpagnoux : s'il faut retravailler le commit, n'hésite pas à me diriger vers la bonne pratique ;-)

@fpagnoux
Copy link
Member

fpagnoux commented Feb 27, 2017

Si par le commit tu veux dire le contenu du code, je ne vois pas de helper de haut niveau qui pourrait satisfaire le besoin, ni même de helper qui pourrait nous retourner les douze mois à partir de l'année, donc la solution proposée ici avec le commentaire fait l'affaire.

Concernant l'historique, la branche est un peu en retard vis à vis de master. Un git rebase origin/master devrait te permettre sans difficulté de la remettre à jour.

Quand je vois les 4 commits, je me dis aussi qu'on pourrait tous les compresser en un seul commit, vu que les 3 derniers sont des correctifs le premier. Tu peux le faire avec git rebase origin/master --interactive ou avec gitup si tu es sous Mac (plus simple). Mais c'est un usage plus avancé, donc pas d'obligation.

Concernant le changelog, on peut essayer d'appliquer les nouvelles guidelines :


13.2.1 — #687

  • Évolution du système socio-fiscal
  • Périodes concernées : Jusqu'au 31/12/2015.
  • Zones impactées :
    • revenus/activite/salarie
  • Le calcul de l'indicatrice de travail à temps plein ppe_tp_sa renvoyait une valeur érronée, provoquant des erreurs dans le calcul de la prime pour l'emploi ppe.

@benjello n'hésite pas à me corriger sur le fond.
@MattiSG, je n'oublie rien ? On est dans un cas où d'après moi le chemin vers le fichier du modèle n'apporte pas d'information claire. On a l'impression que ça concerne tous les salariés, en fait on est dans le domaine spécifique du calcule de la PPE.

@fpagnoux fpagnoux assigned benjello and unassigned fpagnoux Feb 27, 2017
@MattiSG
Copy link
Member

MattiSG commented Feb 27, 2017

git rebase origin/master

git fetch --prune && git rebase origin/master 🙂

Le nouveau changelog me semble très bien (attention à l'accent en trop sur erronée). C'est encore un peu tôt pour avoir des conclusions sur ce qui est clair ou non, je pense :) prenons note et accumulons un ou deux autres exemples. Comment imaginerais-tu (imagineriez-vous) fournir une information utile en ce qui concerne les zones impactées, i.e. qui me permette de déterminer en tant que réutilisateur si je dois creuser cette modification plus avant ou non ?

ProTip section

les compresser en un seul commit

Pour info, on appelle cette opération un squash. Et, si lorsque tu crées un commit de correction tu commit avec git commit --fixup "Message du commit à corriger", la commande git rebase origin/master --autosquash le fera pour toi ! 😄 Plus d'infos. 🌟

@benjello
Copy link
Member Author

benjello commented Mar 3, 2017

@fpagnoux : this PR should be OK now

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.

GTM (j'ai enlevé l'upstream merge)

@benjello benjello merged commit 06a37a5 into master Mar 6, 2017
@benjello benjello deleted the fix-ppe-tp-sa branch March 6, 2017 12:04
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.

4 participants