-
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
Teste contre la version build #1178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documente le "pourquoi", afin que ça ne soit pas détricoté par erreur plus tard.
6e27263
to
c7a209a
Compare
c7a209a
to
f3a1a6c
Compare
f3a1a6c
to
aa32ac4
Compare
J'ai fait un test, ça fonctionne sur la CI (le test échoue comme attendu). |
SVP rebase + fixup avant de merger Edit: done |
f3024ee
to
831302b
Compare
831302b
to
17569f3
Compare
52dc391
to
ccebdfe
Compare
767c6ce
to
9ae00bf
Compare
Up ? @fpagnoux il me semble que les réserves que tu as soulevées sont résolues ? |
745ea4c
to
43f8de7
Compare
43f8de7
to
2d2c996
Compare
251f318
to
a597f2f
Compare
L'idée est d'éviter de faux positifs lors du testing, dans les cas où on ajoute d'assets mais qu'on oublie de les déclarer dans le fichier `MANIFEST.in`
L'idée est d'éviter d'erreurs involontaires lors de la modification de cette librairie.
À partir du moment où nous allons tester contre la version packaged de la librairie, il faut qu'elle soit exclue du cache de dépendances.
a597f2f
to
72ab796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne suis pas à l'aise qu'un diff qui change le Makefile soit embarqué dans un commit appelé "Modifie Changelog et setup.py", peux-tu le séparer et documenter l'intention de ces changements?
@Morendil Je l'ai déjà fait, et ajouté dans la description du commit concerné 😃 |
CHANGELOG.md
Outdated
### 29.2.1 [#1178](https://github.com/openfisca/openfisca-france/pull/1178) | ||
|
||
* Changement mineur | ||
* Périodes concernées : toutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La spécification des périodes n'est pertinente qu'en cas de changement dans la législation. Elle ne doit pas être spécifiée pour des changements techniques.
MANIFEST.in
Outdated
recursive-include openfisca_france/reforms/parameters * | ||
recursive-include openfisca_france/scripts * | ||
recursive-include openfisca_france/situation_examples * | ||
recursive-include openfisca_france * | ||
include openfisca_france/tests/__init__.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ces fichiers n'existent plus depuis un certain temps. J'ai plusieurs fois pushé des commit sur cette branche pour supprimer les trois lignes qui suivent, mais à chaque fois mes commits ont sauté, probablement lors d'un push --force
. Utiliser --force-with-lease
à la place de --force
permet dans la majorité des cas d'éviter ce problème (merci @pblayo pour l'astuce 🙂 ), essayons de prendre pour habitude de l'utiliser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and tested.
Pour info, on peut tester la publication sur France de la même manière que sur les autres paquets, en désactivant en plus temporairement les tests pour éviter les longs
Je ne peux pas merger car @Morendil a des requested changes, mais please pas d'édition de la config circle, à moins que vous ne vous engagiez à ré-effectuer tous les tests 🙂
8b8179c
to
e2383fc
Compare
Child of #1177
wheel
Ces changements :
Quelques conseils à prendre en compte :
Documentez votre contribution avec des références législatives.setup.py
.CHANGELOG.md
.