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

Améliorations du code (via les erreurs de landscape.io) #2188

Merged
merged 2 commits into from Feb 3, 2015

Conversation

Projects
None yet
6 participants
@gustavi
Copy link
Member

commented Jan 30, 2015

Q R
Correction de bugs ? oui
Nouvelle Fonctionnalité ? non
Tickets concernés x

Tous ces problèmes ont été signalés par landscape.io.

QA

Regarde si rien n'est choquant et attendre que les tests passent.

@gustavi gustavi force-pushed the gustavi:health branch to 03b15a9 Jan 30, 2015

@gustavi gustavi force-pushed the gustavi:health branch from 03b15a9 to 1541ada Jan 31, 2015

@gustavi

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2015

Mis à jour.

@landscape-bot

This comment has been minimized.

Copy link

commented Jan 31, 2015

Code Health
Repository health increased by 4% when pulling 1541ada on gustavi:health into 9870e70 on zestedesavoir:dev.

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2015

Coverage Status

Coverage decreased (-0.01%) to 75.48% when pulling 1541ada on gustavi:health into 9870e70 on zestedesavoir:dev.

@@ -10,7 +10,6 @@
except ImportError:
import json as json_reader
import json
import json as json_writer

This comment has been minimized.

Copy link
@pierre-24

pierre-24 Feb 1, 2015

Member

Cette correction est plus esthétique qu'autre chose, en gros. Elle me choque pas, mais je préfère demander l'avis de @firm1 sur celle là, vu que c'est de lui.

This comment has been minimized.

Copy link
@gustavi

gustavi Feb 1, 2015

Author Member

Bah cet import est totalement inutile.

This comment has been minimized.

Copy link
@firm1

firm1 Feb 2, 2015

Contributor

l'alias json_writer est surtout là pour des raisons de prévision d'optimisations. Pour le reader par exemple on utilise la lib ujson plus rapide en lecture que la lib standard. Il s'avère que la lib la plus rapide en écriture qui fait ce qu'on veut (indentation) est celle native. Mais ça ne sera pas forcément toujours le cas au fil des mises à jour, et donc si jamais simplejson ou usjon gagne en perf d'ici là, il sera toujours plus simple d'appliquer les modif dans l'import plutot que d'aller modifier un peut partout dans le code.

Aujourd'hui c'est inutile (et innofensif), mais demain, ça peut s'averer dangereusement avantageux.

This comment has been minimized.

Copy link
@gustavi

gustavi Feb 3, 2015

Author Member

Oui mais c'est débile d'avoir 2 imports identiques. Soit on appelle tout json, soit tout json_writter.

This comment has been minimized.

Copy link
@firm1

firm1 Feb 3, 2015

Contributor

Bah non puisqu'une lib peut être meilleure en lecture et l'autre meilleure
en écriture. Si tu appelles tout avec le même nom, comment tu fais pour
dire "je veux lire a ujson et écrire avec simplejson" ?

Le mar. 3 févr. 2015 15:33, Laville Augustin notifications@github.com a
écrit :

In zds/article/views.py
#2188 (comment)
:

@@ -10,7 +10,6 @@
except ImportError:
import json as json_reader
import json
-import json as json_writer

Oui mais c'est débile d'avoir 2 imports identiques. Soit on appelle tout
json, soit tout json_reader.


Reply to this email directly or view it on GitHub
https://github.com/zestedesavoir/zds-site/pull/2188/files#r24006617.

This comment has been minimized.

Copy link
@gustavi

gustavi Feb 3, 2015

Author Member

Je voualis dire json_writter, pardon. Je dis juste que c'est débile d'avoir :

import foo
import foo as bar

This comment has been minimized.

Copy link
@firm1

firm1 Feb 3, 2015

Contributor

ah je ne l'avais pas vu le import foo d'en haut.

Dans ce cas, c'est mieux de garder le import foo as bar et virer l'autre

This comment has been minimized.

Copy link
@gustavi

gustavi Feb 3, 2015

Author Member

Je changerai ça alors.

@pierre-24

This comment has been minimized.

Copy link
Member

commented Feb 1, 2015

Landscape.io en a profité pour encore en retrouver, si j'en crois le message qu'il t'as posté. Du reste, rien ne me choque quand je relis le code.

@gustavi

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2015

Il en reste, je vais tout corriger petit à petit. Là on gagne déjà 4% !

EDIT : on a fait une grosse màj depuis (ajout du support de Django pour de vrai). Je propose de merger ça ASAP pour que je puisse bosser sur les autres erreurs des que possible).

@gustavi

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2015

Mis à jour. À merger ASAP.

SpaceFox added a commit that referenced this pull request Feb 3, 2015

Merge pull request #2188 from gustavi/health
Améliorations du code (via les erreurs de landscape.io)

@SpaceFox SpaceFox merged commit 2554be5 into zestedesavoir:dev Feb 3, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@SpaceFox SpaceFox added this to the Version 1.6 milestone Feb 3, 2015

@gustavi gustavi deleted the gustavi:health branch Feb 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.