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 glob expressions in CI config #1038

Merged
merged 6 commits into from
Jul 13, 2018
Merged

Fix glob expressions in CI config #1038

merged 6 commits into from
Jul 13, 2018

Conversation

guillett
Copy link
Member

@guillett guillett commented Jul 12, 2018

  • Amélioration technique.
  • Zones impactées : .circleci/config.yaml:
  • Détails :
    • Corrige les appels circleci tests glob pour que ** soit effectivement récursif

@guillett guillett requested a review from claireleroy July 12, 2018 13:39
@guillett
Copy link
Member Author

@ClaireLeroyIPP j'ai besoin de toi pour valider les modifications faites sur https://github.com/openfisca/openfisca-france/pull/1038/files#diff-e79521b2f36d9e2e8fc12f9789d0479a :)

CHANGELOG.md Outdated
### 22.1.1 [#1038](https://github.com/openfisca/openfisca-france/pull/1038)

* Amélioration technique.
* Zones impactées : `.circleci/config.yaml`:
Copy link
Member

Choose a reason for hiding this comment

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

Pas besoin de définir une zone impactée pour une amélioration technique.

Ceci dit, si même les contributeurs chevronnées continuent à faire l'erreur, peut être que ce n'est pas assez clair... On pourrait peut-être remplacer "Zones impactées" par "Zones de la législation impactées" 🤔 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Merci ! Du coup, je t'ai mis en reviewer :)

Copy link
Member

@fpagnoux fpagnoux Jul 12, 2018

Choose a reason for hiding this comment

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

Est-ce que tu as un lien vers les résultats de Circle qui montre que grâce à cette modification le nombre de tests exécutés au augmenté?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://circleci.com/gh/openfisca/openfisca-france/2127

J'ai un lien qui compte le nombre de fichiers des deux expressions avec et sans ".

Copy link
Member

Choose a reason for hiding this comment

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

@guillett guillett requested review from fpagnoux and Anna-Livia July 12, 2018 16:21
@@ -4295,7 +4295,7 @@ def formula_2017_01_01(foyer_fiscal, period, parameters):
report_reduc_invest_2016 = f7op + f7oq + f7or + f7os + f7ot

report_reduc_invest_anterieur = (
report_reduc_invest_2010_2011
report_reduc_invest_2009_2010
Copy link
Contributor

Choose a reason for hiding this comment

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

Ces modifications m'ont l'air cool, mais je ne vois pas exactement le lien avec la PR.
Je propose deux options (il y en a peut être d'autres):

  • Faire une PR différente
  • Décrire les modifications effectuées dans le changelog

Copy link
Member Author

Choose a reason for hiding this comment

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

Ces modifications sont nécessaires pour que les tests qui n'étaient pas lancés avant passent.

@guillett guillett requested a review from Anna-Livia July 13, 2018 12:58
@claireleroy
Copy link
Contributor

@guillett Les modifications que tu as faites sur l'IR sont correctes pour moi :)

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.

Merci d'avoir trouvé ça, c'est moche 😬. J'aurais du mieux regarder les outputs de Circle quand on est passé à Circle 2...

@guillett
Copy link
Member Author

J'ai dégradé les performances de 52%.

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.

5 participants