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

Retire la gestion inutilisée de l'i18n #759

Merged
merged 4 commits into from
May 23, 2017
Merged

Conversation

Anna-Livia
Copy link
Contributor

@Anna-Livia Anna-Livia commented May 22, 2017

Connected to #737
Merci de contribuer à OpenFisca ! Effacez cette ligne ainsi que, pour chaque ligne ci-dessous, les cas ne concernant pas votre contribution :)

*Changement mineur.

  • Périodes concernées : toutes.
  • Zones impactées :
    -- In openfisca-france/setup.cfg :
# Babel configuration
[compile_catalog]
domain = openfisca-france
statistics = true

[extract_messages]
add_comments = TRANSLATORS:
copyright_holder = OpenFisca Team
msgid-bugs-address = contact@openfisca.fr
width = 80

[init_catalog]
domain = openfisca-france
input_file = openfisca_france/i18n/openfisca-france.pot
output_dir = openfisca_france/i18n

[update_catalog]
domain = openfisca-france
input_file = openfisca_france/i18n/openfisca-france.pot
output_dir = openfisca_france/i18n
previous = true

-- In openfisca-france/circle.yml , line 12

- pip install babel # this line is only needed by internationalization with i18n. We should remove it when we get rid of i18n.
 - python setup.py compile_catalog  # this line is only needed by internationalization with i18n. We should remove it when we get rid of i18n.

-- In openfisca-france/setup.py, line 26

('share/locale/fr/LC_MESSAGES', ['openfisca_france/i18n/fr/LC_MESSAGES/openfisca-france.mo']),

-- Deleted openfisca-france/i18n

  • Détails :
    • Description de la fonctionalité ajoutée ou du nouveau comportement adopté.
    • Cas dans lesquels une erreur était constatée.

Ces changements (effacez les lignes ne correspondant pas à votre cas) :

  • Modifient des éléments non fonctionnels de ce dépôt (par exemple modification du README).

@Anna-Livia Anna-Livia requested a review from fpagnoux May 22, 2017 11:44
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.

👍 pour le code, bien joué pour avoir trouvé toutes les références !

Juste le changelog à reprendre un peu :)

CHANGELOG.md Outdated

* Changement mineur
* Détails :
- suppression des fichiers `i18n`, ainsi que les références aux `mo (`makefile` et `setup.py`)
Copy link
Member

Choose a reason for hiding this comment

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

Il faut faire attention à ce que le changelog soit toujours écrit d'un point de vue du de l'utilisateur, et qu'il apporte des informations qui soient complémentaires avec le diff.

"Suppression des fichiers i18n" est une information que j'obtiens en regardant le diff. Pareil pour les références aux fichiers modifiés Makefile et setup.py.

Proposition alternative:

  • Suppression de l'internationalisation (traductions des messages d'erreurs). Cette fonctionnalité n'était pas utilisée.

@Anna-Livia Anna-Livia requested a review from fpagnoux May 23, 2017 16:53
@Anna-Livia Anna-Livia merged commit a57e52f into master May 23, 2017
@MattiSG
Copy link
Member

MattiSG commented May 26, 2017

Cool, merci @Anna-Livia ! 🙂

Un peu de feedback sur la gestion des PR.

  1. Penser à effacer la ligne qui dit « Effacez cette ligne ainsi que […] » de la description de la PR 😉
  2. « retirer_i18n#737 » n'est pas un bon nom de PR. L'objectif est de décrire dans l'intitulé ce que fera cette PR si elle est mergée. Un verbe à l'infinitif ou à l'actif est donc approprié. Nous ne référençons pas les numéros d'issue dans le nom de la PR, mais dans son corps : quand on regarde la liste des PR, le numéro de l'issue n'est pas très parlant. capture d ecran 2017-05-26 a 04 55 32
  3. Je ne suis pas convaincu qu'ici il s'agisse d'un « Connected to Retirer le dossier i18n #737 », mais plutôt d'un « Fixes Retirer le dossier i18n #737 ». La question est : l'intégration de cette PR résout-elle l'intégralité de ce qui est décrit dans Retirer le dossier i18n #737 ? Si oui, alors c'est un « Fixes ». Cela automatisera la fermeture de l'issue associée, créera un lien de justification de la fermeture et déplacera la carte dans Waffle 🙂
  4. Lorsqu'une PR a été mergée, il est important de supprimer la branche associée, afin d'éviter qu'elles ne s'accumulent. Tu peux faire cela à la ligne de commande (git push --delete), ou par l'interface graphique de GitHub (dans ce cas, pense bien à ajouter à l'option --prune lorsque tu fais git fetch ou git pull, afin de supprimer les branches locales associées ; pour ma part, j'utilise git drop). capture d ecran 2017-05-26 a 04 58 37

Également, sur les messages de commit :

retirer_i18n#737

Même remarque qu'au-dessus : un verbe à l'actif, une phrase complète. Bref, a great commit message 😉
Si ce commit seul permet de résoudre ce qui a été décrit dans #737, alors tu peux mettre dans le corps du message (et non dans son titre, donc avec une ligne vide) “Fixes #737”.

deletes references to babel and .mo

Mieux, mais avec une majuscule en début de phrase, et à l'infinitif.
L'infinitif est utilisé (et non l'indicatif) par cohérence avec les messages générés automatiquement par Git lors de certaines opérations (Merge X into Y, Revert X…).

Update Changelog and setup.py with version number

Super sur la forme !
En revanche, sur le fond : la liste des fichiers modifiés est déjà lisible dans le diff. Ce qui nous intéresse, et qui t'intéressera quand tu essaieras de comprendre ce que tu as voulu faire quand tu découvriras dans trois mois que ce commit a introduit un bug subtil 😉 c'est pourquoi tu as fait ce changement. En l'occurrence, peut-être est-ce simplement “Bump version number”. Ou peut-être “Document changes” ?
(je prends bien évidemment ce message de commit comme exemple, la formulation n'est pas particulièrement problématique vu le contenu du diff)

Update Changelog following code review

Non : ce message ne décrit pas l'intention. Qu'as-tu fait dans c518c35 ? Était-ce “Improve changelog readability” ? “Write changelog from the user's viewpoint” ? Ou peut-être était-ce simplement un fixup! Update Changelog and setup.py with version number (référence sur fixup) ?
Si tu veux sourcer l'origine du changement, je te recommande de le faire dans le corps du message de commit et non dans son titre :

Write changelog from the user's viewpoint

Following code review

Félicitations pour ces premières contributions ! 😃

@MattiSG MattiSG deleted the retirer_i18n#737 branch May 26, 2017 03:15
@MattiSG MattiSG changed the title retirer_i18n#737 Retire la gestion inutilisée de l'i18n May 26, 2017
@MattiSG MattiSG removed the request for review from fpagnoux May 31, 2017 12:05
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.

3 participants