Lint YAML files #1039

guillett · 2018-07-13T12:01:06Z

Amélioration technique.
Détails :
- Ajout d'un linter pour les fichiers YAML

guillett · 2018-07-13T12:09:20Z

Cette PR est une étape vers la normalisation des fichiers YAML. Il y a des warnings non bloquants pour cette PR mais à traiter avant de pouvoir manipuler systématiquement les fichiers YAML.

* Required to pass ruamel.yaml roundtrip test

* ' false' 840 matches across 259 files * ' False' 84 matches across 20 files * ' true' 500 matches across 276 files * ' True' 186 matches across 24 files

Anna-Livia

Great work on the linting !
I would add a make lint that would run yamllint on test and parameter.

Anna-Livia · 2018-07-16T13:12:07Z

.circleci/config.yml

@@ -23,19 +23,24 @@ jobs:
          name: Install dependencies
          command: |
            pip install --upgrade pip twine wheel
-            pip install --editable .[test] --upgrade
+            pip install --editable .[test,lint] --upgrade


Why not put the linter with the test dependencies ?

Pour apporter de la granularité aux niveaux des dépendances.

Dans quel cas concret cette granularité est-elle souhaitable?
Le package yamllint est-il connu pour être difficile à installer dans certains environnements?
S'agit-il d'un package très lourd qui ralentit significativement l'installation?

Un linter me paraît être typiquement une dépendance de développement, au même titre que nose ou flake8. S'il n'y a pas de raison particulière, je suis pour qu'on le mette dans test, avec les autres dépendances de dev.

(Il y a déjà bien trop d'extra-dépendances sur France.)

Anna-Livia · 2018-07-16T13:13:04Z

.circleci/config.yml

          #  pip install --editable git+https://github.com/openfisca/openfisca-core.git@BRANCH_NAME#egg=OpenFisca-Core  # use a specific branch of OpenFisca-Core

      - save_cache:
          key: v1-py2-{{ checksum "setup.py" }}
          paths:
            - /tmp/venv/openfisca_france

+      - run:
+          name: Lint YAML files


Same, why not put it in the run test part ?

and why not run the linter on all yaml files (expecially the parameters ?)

Also, it seems to me that a test with a unvalid yaml fails with openfisca-run-test. Is this to fail faster ?

Un linter (comme flake8) fait plus que détecter les erreurs de syntaxe, il impose aussi des bonnes pratiques et une consistence qui permettent d'avoir un code plus propre.

Les règles de gestion du yamllintme paraissent trop "relaxées" pour jouer ce rôle de bonne pratiques. C'est pour cela que je posait la question que peut être il était surtout utilisait comme validateur.

@Anna-Livia je ne comprends pas ta remarque, as-tu un lien vers CircleCI ?

Contrairement à ce qu'on peut passer circleci tests glob "**/*.yaml" correspond à ls **/*.yaml et non pas ls tests/**/*/yaml. L'outil fourni par CircleCI est initialement fait pour les tests ; c'est de là que vient le préfix circleci tests dans les commandes.

J'espérais que cette PR soit mergée dans la semaine et itérer sur une nouvelle base cette semaine. yamllint génére aujourd'hui beaucoup d'avertissements qui restent à être gérés. Une fois les modifications nécessaires apportées aux fichiers YAML les problèmes de format généreront des erreurs et non plus des avertissements.

Anna-Livia · 2018-07-16T13:13:51Z

.circleci/config.yml

      - run:
          name: Run tests
          command: |
            nosetests `circleci tests glob "tests/**/*.py" | circleci tests split`
-            openfisca-run-test `circleci tests glob "tests/**/*.{yaml,yml}" | circleci tests split`
+            openfisca-run-test `circleci tests glob "tests/**/*.yaml" | circleci tests split`


Is it me or do we not pass flake8 on circle ci ? (cc @fpagnoux )

Pas sur OpenFisca-France. Personne n'a eu la motivation/volonté pour ajuster la configuration flake et corriger les 3644 erreurs flake présentes aujourd'hui.

fpagnoux · 2018-07-16T20:16:30Z

.circleci/config.yml

      - run:
          name: Run tests
          command: |
            nosetests `circleci tests glob "tests/**/*.py" | circleci tests split`
-            openfisca-run-test `circleci tests glob "tests/**/*.{yaml,yml}" | circleci tests split`


Quel avantage à ne plus gérer les fichiers avec une extension .yml ? Il n'y a pas un risque qu'un contributeur en créé un par accident?

J'oublie toujours la forme que doit prendre l'expression régulière qui effectue un OU.
Ne pas gérer les deux c'est simplifier.

Le risque qu'un contributeur créé un fichier de test en .yml et que ça ne soit pas détecté à la revue (qui regarde les extensions?) me paraît loin d'être anecdotique.

La simplification de cette ligne, plutôt compréhensible à la lecture, me paraît un gain léger comparé aux problèmes que pourraient poser la présence de tests non exécutés.

Anna-Livia · 2018-07-17T15:03:59Z

Pour info, en faisant passer le script de performance (et en augmentant la standard deviationà 13 (la denière PR sur France qui augmente le nombre de test) à augmenté sensiblemement le temps d'exécution ) donne ceci :

python  openfisca_france/scripts/performance_tests/test_circleci_builds.py 2209 2208 

WARNING:root:The master branch has 13 builds available for analysis.
INFO:root:current build = 314555.5 ; mean is = 230761.61538461538
This build makes the test performance more than 13% worst : 36.31188162542698%

guillett · 2018-07-23T07:44:15Z

Merci @Anna-Livia et @fpagnoux pour vos commentaires.
Pouvez-vous signaler vos remarques bloquantes :)

fpagnoux

Points bloquants pour moi:

Retrait du support des fichiers .yml
Ajout d'une extra dépendance

guillett force-pushed the lint branch from ab1752a to acc09de Compare July 13, 2018 12:03

guillett requested review from fpagnoux and Anna-Livia July 13, 2018 14:40

guillett added 7 commits July 13, 2018 16:43

Add lint extra requirement

819f809

Activate YAML linter in CI

3f00c1e

Refactor YAML lint config

7f0a705

Remove duplicated keys in YAML test files

cd76a34

Reformat .xxx in 0.xxx

7f8bc75

* Required to pass ruamel.yaml roundtrip test

Change boolean representation in YAML

b279a5b

* ' false' 840 matches across 259 files * ' False' 84 matches across 20 files * ' true' 500 matches across 276 files * ' True' 186 matches across 24 files

Add missing new lines

a075d0c

guillett force-pushed the lint branch from acc09de to 6a325a0 Compare July 13, 2018 14:45

guillett added 6 commits July 13, 2018 17:02

Remove trailing spaces in YAML files

2062973

Fix comma warnings in YAML lint

b52873f

Fix colons warnings in YAML lint

364a31e

Fix empty-lines warnings in YAML lint

c492958

Update version and CHANGELOG

5d48157

Remove warnings in yamllint config

0d659b6

guillett force-pushed the lint branch from 6a325a0 to 0d659b6 Compare July 13, 2018 15:02

guillett added 2 commits July 13, 2018 17:58

Split lint work by container

c446f64

No longer support .yml files

e79a553

Anna-Livia reviewed Jul 16, 2018

View reviewed changes

Anna-Livia added the flow:contrib:doing label Jul 16, 2018

Anna-Livia self-assigned this Jul 16, 2018

fpagnoux reviewed Jul 16, 2018

View reviewed changes

fpagnoux requested changes Jul 23, 2018

View reviewed changes

Restore .yml support and merge lint to test extra requirement

447c787

guillett requested a review from fpagnoux July 23, 2018 19:17

fpagnoux approved these changes Jul 23, 2018

View reviewed changes

guillett merged commit e8ddf14 into master Jul 24, 2018

guillett deleted the lint branch July 24, 2018 08:19

guillett mentioned this pull request Aug 27, 2018

Normalize test files in YAML #991

Closed

bonjourmauko mentioned this pull request Sep 25, 2018

Linte les fichiers YAML/Python en incrémental #1135

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint YAML files #1039

Lint YAML files #1039

guillett commented Jul 13, 2018

guillett commented Jul 13, 2018

Anna-Livia left a comment

Anna-Livia Jul 16, 2018

guillett Jul 23, 2018

fpagnoux Jul 23, 2018

Anna-Livia Jul 16, 2018

Anna-Livia Jul 16, 2018

Anna-Livia Jul 16, 2018

fpagnoux Jul 16, 2018

Anna-Livia Jul 17, 2018

guillett Jul 23, 2018

guillett Jul 23, 2018 •

edited by fpagnoux

Loading

guillett Jul 23, 2018

Anna-Livia Jul 16, 2018

fpagnoux Jul 16, 2018

fpagnoux Jul 16, 2018

guillett Jul 23, 2018

fpagnoux Jul 23, 2018 •

edited

Loading

Anna-Livia commented Jul 17, 2018 •

edited by fpagnoux

Loading

guillett commented Jul 23, 2018

fpagnoux left a comment

Lint YAML files #1039

Lint YAML files #1039

Conversation

guillett commented Jul 13, 2018

guillett commented Jul 13, 2018

Anna-Livia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillett Jul 23, 2018 • edited by fpagnoux Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Anna-Livia commented Jul 17, 2018 • edited by fpagnoux Loading

guillett commented Jul 23, 2018

fpagnoux left a comment

Choose a reason for hiding this comment

guillett Jul 23, 2018 •

edited by fpagnoux

Loading

fpagnoux Jul 23, 2018 •

edited

Loading

Anna-Livia commented Jul 17, 2018 •

edited by fpagnoux

Loading