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

Nettoyage #1505

Merged
merged 48 commits into from Sep 17, 2014
Merged

Nettoyage #1505

merged 48 commits into from Sep 17, 2014

Conversation

cgabard
Copy link
Contributor

@cgabard cgabard commented Sep 12, 2014

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

Cette PR est un début de nettoyage de notre basse de code. C'est chiant a faire (et a reviewer et QAiser) mais vu la taille du projet, il faut bien le faire.

L'idée est de repasser sur les modules et de les nettoyer, un par un. Cela comprend :

  • relecture/correction du code
  • factorisation quand c'est possible
  • amélioration de la gestion des erreurs
  • ajout de doc-string
  • simplification quand possible
  • rajout de tests
  • rajout de doc

J'ai donc ouvert le bal avec les 4 premiers modules dans zds/utils/templatetags. Il n'y a pas de changement fonctionnels à de rares exceptions pret (ajout de cas de gestions d'erreur, correction de bugs au passage), le but est juste de rendre ces modules plus propre.

Je suis donc repassé sur ces 4 modules, mit au propre, commenté, documenté et rajouté des tests. Si ça convient, je continurait quand j'aurais le temps.

Au passage j'ai corrigé quelques probs de mise en forme dans la doc.

J'ai essayé de tester le max de trucs, erreurs comprises. Mais certaines choses ne sont pas facile a tester. Je les ai notés dans les fichiers de tests pour une passe ultérieur.

Note pour QA

Désolé, ça va être aussi chiant pour vous que pour moi. Ces fichiers étant liés aux templates, il va falloir regarder un peu partout. Mais en particulier :

  • Que les liens générés automatiquement (message suivant sur le forum et pagination, etc.) sont encore corrects.
  • Que les dates sont toujours formatés pareils
  • Que le markdown continue a être correctement formaté
  • Que l'ensemble du site fonctionne.

Vu l'impacte, ce serait peut être bien que ce soit quelqu'un du front ( @Alex-D ?) qui s'en occupe. J'ai touché aux tags append_to_get, captureas, format_date, tooltip_date, humane_time, emarkdown, emarkdown_inline, et les decale_header_N.

J'apprécierai aussi une relecture technique. Les commits sont nombreux mais petits et donc facile à suivre.

{% load profile %}
{% load date %}
{% load thumbnail %}
{% load date %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu charges 2 fois date ici.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

en effet j'ai pas fait gaffe qu'il était déjà chargé,

@@ -1,6 +1,7 @@
# coding: utf-8
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Je pense que le bonne usage oblige à le mettre, mais on le fait pas de manière systématique. Un commentaire sur le sujet ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bah, c'est une bonne pratique de le mettre donc je complète quand je passe dessus et j'y pense. Le fait que ce ne soit pas systématique importe peu je pense : autant le faire si on y pense. Mais c'est pas grave si c'est n'est pas le cas. En attendant, comme la doc et les tests, on ne va pas se plaindre si c'est fait meme si ce n'est pas systématique :)

@pierre-24
Copy link
Member

Ton fail provient d'erreurs pep8, donc je pense que c'est "facile" a régler. Pour le reste, j'ai pas vu d'erreur en lisant le code.

@cgabard
Copy link
Contributor Author

cgabard commented Sep 16, 2014

Ok normalement ça devrait passer maintenant :) et j'ai résolut les conflit avec dev.

Pret pour QA

@pierre-24
Copy link
Member

Bon, il a fallu que tu tombes sur une erreur npm, sinon c'est pas drôle --"

@Alex-D Alex-D added C-Back Concerne le back-end Django Facile Bon ticket pour débuter pour rejoindre le développement ! labels Sep 16, 2014
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling da96e7a on cgabard:clean_templatetags into * on zestedesavoir:dev*.

@firm1
Copy link
Contributor

firm1 commented Sep 17, 2014

J'ai passé le code en revue, et je ne vois rien de choquant. ça me parait bon, sosu réserve de vérifier l'affichage de pages impactées.

@pierre-24
Copy link
Member

... Et de faire un bon rebase.

@cgabard
Copy link
Contributor Author

cgabard commented Sep 17, 2014

... Et de faire un bon rebase.

Pourquoi ? J'ai rebasé hier (enfin merger dev dans la branche) et il me dit qu'on peut merger sans problème.

@pierre-24
Copy link
Member

Sorry, j'avais mal lu ;)

Rapport de QA : ok à priori. j'ai été me promener sur les pages que je pense impactées sans voir de réel problème. À tester en préprod' sérieusement, mais à merger :)

SpaceFox added a commit that referenced this pull request Sep 17, 2014
@SpaceFox SpaceFox merged commit 4f4ccc3 into zestedesavoir:dev Sep 17, 2014
@Eskimon
Copy link
Contributor

Eskimon commented Sep 17, 2014

:( Ca pouvait pas attendre le merge de "desinscription" ?

@SpaceFox
Copy link
Contributor

Techniquement si, mais il faut demander s'il y a des incompatibilités :(

@SpaceFox SpaceFox added this to the Version 1.1 milestone Sep 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django Facile Bon ticket pour débuter pour rejoindre le développement !
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants