Rend le code des tutos plus pythonique #4772
Conversation
zds/__init__.py
Outdated
| import simplejson as json_reader | ||
| except ImportError: | ||
| import json as json_reader | ||
| import json as json_writer |
There was a problem hiding this comment.
Je ne vois pas l’intérêt d’avoir un json_reader et un json_writer qui font exactement la même chose.
Il n’y a aucun risque d’avoir des conflits avec le module json de la bibliothèque standard parce qu’on fait partout un from zds.
There was a problem hiding this comment.
je ne l'ai jamais compris non plus mais comme ceux qui on mis en place cette distinction depuis casi le début du code ne sont plus actifs sur le dev, on se les traine sans trop se poser de questions.
There was a problem hiding this comment.
C’est vrai. On changera ça plus tard, dans une autre PR au pire.
There was a problem hiding this comment.
Bon, du coup c’est pas grave mais cette PR ne corrige pas complètement #4759 (à mon avis).
There was a problem hiding this comment.
en fait c'est surtout que je pense que créer un fichier pour ça ne sert à rien. Je pense clairement que mon fix est plus logique. Après si tu as des arguments pour tenir la méthodes que tu préconises dans le ticket, je prends.
There was a problem hiding this comment.
Alors oui, je n'ai pas pensé à mettre ça dans __init__.py et j'avoue que c'est mieux 🙃 Je parlais de la différence entre json_reader et json_writer qui est complètement inutile à mon avis (je pense qu'on pourrait rassembler et renommer tout ça json).
There was a problem hiding this comment.
j'ai nommé ça en json_handler comme ça pas de confusion possible.
| title = factory.Sequence(lambda n: 'Mon contenu No{0}'.format(n)) | ||
| description = factory.Sequence(lambda n: 'Description du contenu No{0}'.format(n)) | ||
| title = factory.Sequence('Mon contenu No{0}'.format) | ||
| description = factory.Sequence('Description du contenu No{0}'.format) |
There was a problem hiding this comment.
Attention avec le nombre de paramètres quand-même. Je ne connais pas trop Python, mais en JS, [1, 2, 3].map(someFunction) n’est pas l’équivalent de [1, 2, 3].map(n => someFunction(n)) (et c’est source de bugs bien traîtres).
There was a problem hiding this comment.
sauf que contrairement à js, s'il y avait deux arguments passés via Sequence python aurait crashé car il n'y a pas de "trou noir undefined" si tu passes deux arguments à une fonction qui n'en accepte qu'un ça crash.
There was a problem hiding this comment.
Pas faux. En fait, je me suis dit qu’il y avait peut-être un truc vu qu’ils utilisent une lambda dans la doc de factory-boy. Mais c’était peut-être simplement pour essayer de faire comprendre aux gens ce qu’il se passait.
zds/tutorialv2/tests/tests_lists.py
Outdated
| def setUp(self): | ||
|
|
||
| self.overriden_zds_app = overridden_zds_app |
zds/tutorialv2/tests/tests_feeds.py
Outdated
|
|
||
| def setUp(self): | ||
|
|
||
| settings.overriden_zds_app = overridden_zds_app |
artragis
force-pushed
the
fix_code_pythonicite
branch
from
dbad503
to
4764d3d
Compare
motet-a
added
C-Back
| shutil.rmtree(settings.MEDIA_ROOT) | ||
| if os.path.isdir(overridden_zds_app['content']['extra_content_watchdog_dir']): | ||
| shutil.rmtree(overridden_zds_app['content']['extra_content_watchdog_dir']) | ||
| super().tearDown() |
There was a problem hiding this comment.
Si je ne dis pas de bêtises, ceci va casser si la classe du test hérite de plus d’un mixin qui possède un attribut tearDown, non ?
There was a problem hiding this comment.
Non, s'il y a plus d'un tearDown, il exécutera les teardown de toutes les classes : https://stackoverflow.com/questions/24166225/python3-behaviour-of-super-on-multi-inheritance
artragis
force-pushed
the
fix_code_pythonicite
branch
4 times, most recently
from
1cdf2e0
to
86718d2
Compare
artragis
force-pushed
the
fix_code_pythonicite
branch
from
86718d2
to
1915d3f
Compare
et j'en profite pour régler le problème de l'import json. Pour ce qui est des tests, j'ai commencé à travailler à les factoriser un peu pour qu'ils soient plus lisibles et donc plus maintenables et fiables. Pour l'instant j'ai factorisé que le tearDown mais j'ajouterai des choses après.
artragis
force-pushed
the
fix_code_pythonicite
branch
from
1915d3f
to
58755f8
Compare
|
Belle simplication du code 👍 |
|
Pas mal 🙃 Mais encore une fois, je pense qu’on pourrait très bien renommer |
zds/tutorialv2/tests/__init__.py
Outdated
|
|
||
| class TutorialTestMixin: | ||
| def clean_media_dir(self): | ||
| if os.path.isdir(self.overridden_zds_app['content']['repo_private_path']): |
There was a problem hiding this comment.
En fait on peut enlever tous ces if et utiliser shutil.rmtree(path, ignore_errors=True). C’est un poil plus sûr parce que c’est atomique, et c’est même plus rapide, ça évite de stat(2) pour rien.
There was a problem hiding this comment.
C'est modifié, je vous laisse squash mes commits.
|
@motet-a j'aime pas l'idée de faire d'un côté un travail d'abstraction et d'un autre lui donner le nom d'un des modules qu'on abstrait. Peut être mon côté java/C# qui sort trop. |
En reprenant mon travail sur les API je me suis rendu compte que les tests étaient vraiment moches,
en regardant sur codacy, j'ai vu qu'en effet ça causait pas mal de problèmes donc j'ai décidé de commencer à refactorer.
Pour éviter que les changements ne soient trop grands, je n'ai pour l'instant fait que le tearDown mais par la suite je compte ajouter des méthodes utiles à la création de tests.
Notons que j'en ai profité pour fixer #4759