Review du code par Fomys en mp sur Discord #29

New Issue

Closed

opened 2019-09-09 10:23:47 +02:00 by Eragon · 8 comments

Eragon commented 2019-09-09 10:23:47 +02:00

Member

Copy Link

Fomys m'a fait une review du code hier soir. Je vais donc poster ce qu'il à dit ici:

class VehiculeType(db.Model):
    __tablename__ = 'vehiculetype' 
    id = db.Column(db.Integer, primary_key=True) 
    service_id = db.Column(db.Integer, db.ForeignKey('service.id')) 
    name = db.Column(db.String(400), nullable=False) 
    sigle = db.Column(db.String(20), nullable=False) 
    unlock_level = db.Column(db.Integer, nullable=False) 
    price = db.Column(db.Integer, nullable=False) 
    image = db.relationship('VehiculeImage', secondary='images_vehicules', backref=db.backref('vehiculetype', lazy='dynamic')) 
    service = db.relationship('Service') 
    def __repr__(self): 
        return str(self.name) 
    def __str__(self): 
        return str(self.name) 
class Image(db.Model): 
    id = db.Column(db.Integer, primary_key=True) 
    name = db.Column(db.String(64), unique=True, nullable=False) 
    filename = db.Column(db.String(128), unique=True) 
    def __repr__(self): 
        return self.name 
    @property 
    def url(self): 
        return images.url(self.filename) 
    @property 
    def filepath(self): 
        if self.filename is None: 
            return 
        return images.path(self.filename) 
class VehiculeImage(Image): 
    __tablename__ = 'vehiculeimage' 
class InterventionImage(Image): 
    __tablename__ = 'interventionimage' 

@event.listens_for(Image, 'after_delete')
def del_image(mapper, connection, target): 
    if target.filepath is not None: 
        try: 
            os.remove(target.filepath) 
        except OSError: pass

Fomys m'a fait une review du code hier soir. Je vais donc poster ce qu'il à dit ici:  ```python class VehiculeType(db.Model): __tablename__ = 'vehiculetype' id = db.Column(db.Integer, primary_key=True) service_id = db.Column(db.Integer, db.ForeignKey('service.id')) name = db.Column(db.String(400), nullable=False) sigle = db.Column(db.String(20), nullable=False) unlock_level = db.Column(db.Integer, nullable=False) price = db.Column(db.Integer, nullable=False) image = db.relationship('VehiculeImage', secondary='images_vehicules', backref=db.backref('vehiculetype', lazy='dynamic')) service = db.relationship('Service') def __repr__(self): return str(self.name) def __str__(self): return str(self.name) class Image(db.Model): id = db.Column(db.Integer, primary_key=True) name = db.Column(db.String(64), unique=True, nullable=False) filename = db.Column(db.String(128), unique=True) def __repr__(self): return self.name @property def url(self): return images.url(self.filename) @property def filepath(self): if self.filename is None: return return images.path(self.filename) class VehiculeImage(Image): __tablename__ = 'vehiculeimage' class InterventionImage(Image): __tablename__ = 'interventionimage' @event.listens_for(Image, 'after_delete') def del_image(mapper, connection, target): if target.filepath is not None: try: os.remove(target.filepath) except OSError: pass ```  

image.png

133 KiB

image.png

155 KiB

image.png

18 KiB

Darks commented 2019-09-09 13:06:44 +02:00

Owner

Copy Link

Merci pour la review, même si j'aurai préféré l'avoir textuellement. Cliquer sur des liens dans une image c'est chiant. Et ça bouffe 150× plus de place que du texte. M'enfin, aujourd'hui la mode est à prendre une photo de ses pieds pour envoyer un message « tu fé koi », donc je suppose que c'est une évolution des usages. 😅

1. Faut bien foutre la config quelque part, donc pour le moment ça reste dans config.py. On changera plus tard si besoin. Si y'a mieux, je prend.
2. On n'utilise ni l'un, ni l'autre. Donc si ça tenait qu'à moi j'aurais déjà dégagé les deux package manager.
3. Cf ci-dessus
4. Ce dossier sert à stocker des assets, à savoir des fichiers qui servent de référence pour le développement, mais qui n'ont pas de but fonctionnel. Ça dégagera quand on sera en prod.
5. Ça c'est pour changer le message du login manager. De la config quoi, mais qui intervient après l'instanciation de l'objet. Si y'a un meilleur endroit, je prend.
6. Les import à la fin, c'est pour initialiser les routes et tout le bordel. À savoir qu'il me parait plus logique d'initialiser l'environnement puis de charger les routes. Après c'est un point de vue.
7. Comme expliqué, les fichiers sont là que pour initialiser la bdd
8. Idem
9. Idem
10. Idem
11. Pour les pièces-jointes (images ou non), il faudra en effet gérer ça dans la bdd. Pour les avatars, on en a un seul par membre, donc y'a clairement pas besoin de ça. Le serveur HTTP essaie de fournir /avatar/<id>, si il n'y arrive pas il fournit /avatar/default.png. C'est juste à nous de mettre l'avatar dans le bon dossier et de configurer nginx pour que ça fonctionne.
12. Pour les trophées, c'est pas toujours compter des objets. Donc quoi qu'il arrive y'a du code à hardcoder, et donc autant le mettre à un seul endroit pour pas que ce soit éparpillé partout.
13. Je vais voir les blueprints
14. Oh, ça peut être intéressant en effet. Même si on a des trucs à modifier (envoyer des notifs quand on modifie un membre par exemple)
15. Ouaip, ça c'est juste une fonction qui affiche « Oui » si c'est un trophée et « Non » sinon. Utilisée dans ce cadre : Le trophée {{ trophy.name }} est un trophée : {{ trophy|is_trophy }}. Je reconnais que c'est pas spécialement utile, mais ça évite de mettre des conditions à chaque fois.
16. Je sais pas. On n'a pas besoin de Babel, et Jinja propose pas exactement cette fonction je crois.
17. À voir. C'est clairement pas une priorité.

Je note qu'il faut se pencher sur 13, 14 et 17. Si il a des propositions pour le reste, on peut en discuter ici (ou sur la shout dev à la rigueur).

L'inscription est gratuite, on ne partage pas les données avec qui que ce soit, donc pas de soucis pour quitter Discord. 😛

Merci pour la review, même si j'aurai préféré l'avoir textuellement. Cliquer sur des liens dans une image c'est chiant. Et ça bouffe 150× plus de place que du texte. M'enfin, aujourd'hui la mode est à prendre une photo de ses pieds pour envoyer un message « tu fé koi », donc je suppose que c'est une évolution des usages. :sweat_smile: 1. Faut bien foutre la config quelque part, donc pour le moment ça reste dans `config.py`. On changera plus tard si besoin. Si y'a mieux, je prend. 2. On n'utilise ni l'un, ni l'autre. Donc si ça tenait qu'à moi j'aurais déjà dégagé les deux package manager. 3. Cf ci-dessus 4. Ce dossier sert à stocker des assets, à savoir des fichiers qui servent de référence pour le développement, mais qui n'ont pas de but fonctionnel. Ça dégagera quand on sera en prod. 5. Ça c'est pour changer le message du login manager. De la config quoi, mais qui intervient *après* l'instanciation de l'objet. Si y'a un meilleur endroit, je prend. 6. Les import à la fin, c'est pour initialiser les routes et tout le bordel. À savoir qu'il me parait plus logique d'initialiser l'environnement *puis* de charger les routes. Après c'est un point de vue. 7. Comme expliqué, les fichiers sont là que pour initialiser la bdd 8. Idem 9. Idem 10. Idem 11. Pour les pièces-jointes (images ou non), il faudra en effet gérer ça dans la bdd. Pour les avatars, on en a un seul par membre, donc y'a clairement pas besoin de ça. Le serveur HTTP essaie de fournir `/avatar/<id>`, si il n'y arrive pas il fournit `/avatar/default.png`. C'est juste à nous de mettre l'avatar dans le bon dossier et de configurer nginx pour que ça fonctionne. 12. Pour les trophées, c'est pas toujours compter des objets. Donc quoi qu'il arrive y'a du code à hardcoder, et donc autant le mettre à un seul endroit pour pas que ce soit éparpillé partout. 13. Je vais voir les blueprints 14. Oh, ça peut être intéressant en effet. Même si on a des trucs à modifier (envoyer des notifs quand on modifie un membre par exemple) 15. Ouaip, ça c'est juste une fonction qui affiche « Oui » si c'est un trophée et « Non » sinon. Utilisée dans ce cadre : `Le trophée {{ trophy.name }} est un trophée : {{ trophy|is_trophy }}`. Je reconnais que c'est pas spécialement utile, mais ça évite de mettre des conditions à chaque fois. 16. Je sais pas. On n'a pas besoin de Babel, et Jinja propose pas exactement cette fonction je crois. 17. À voir. C'est clairement pas une priorité. Je note qu'il faut se pencher sur 13, 14 et 17. Si il a des propositions pour le reste, on peut en discuter **ici** (ou sur la shout `dev` à la rigueur). L'inscription est gratuite, on ne partage pas les données avec qui que ce soit, donc pas de soucis pour quitter Discord. :stuck_out_tongue:

Eragon commented 2019-09-09 13:25:29 +02:00

Author

Member

Copy Link

Merci pour la review, même si j’aurai préféré l’avoir textuellement. Cliquer sur des liens dans une image c’est chiant. Et ça bouffe 150× plus de place que du texte. M’enfin, aujourd’hui la mode est à prendre une photo de ses pieds pour envoyer un message « tu fé koi », donc je suppose que c’est une évolution des usages.

Je m'en doute bien j'ai hésité, mais coier-coller des choses venant de discord est immonde et hyper long à refaire.

2. Tu ne les utilise pas, Lephé utilise virtualenv et moi pipenv x)

Le site n'étant pas très responsive(la v42) il à préféré utiliser discord depuis son tel. je lui passe le lien de cette issue, il viendra si il en à envie/le temps.

> Merci pour la review, même si j’aurai préféré l’avoir textuellement. Cliquer sur des liens dans une image c’est chiant. Et ça bouffe 150× plus de place que du texte. M’enfin, aujourd’hui la mode est à prendre une photo de ses pieds pour envoyer un message « tu fé koi », donc je suppose que c’est une évolution des usages. Je m'en doute bien j'ai hésité, mais coier-coller des choses venant de discord est immonde et hyper long à refaire. 2. Tu ne les utilise pas, Lephé utilise virtualenv et moi pipenv x) Le site n'étant pas très responsive(la v42) il à préféré utiliser discord depuis son tel. je lui passe le lien de cette issue, il viendra si il en à envie/le temps.

Lephenixnoir commented 2019-09-09 13:28:03 +02:00

Owner

Copy Link

Euh... c'est gentil de sa part, et je vais définitivement regarder individuellement ces remarques.

Cela dit c'est pas orthodoxe d'évaluer le travail existant avant d'avoir prouvé ses propres compétences, donc je n'irai pas forcément chercher très loin quand j'estimerai devoir réfuter les remarques.

1. Oui c'est exact, on ne s'en sert pas actuellement, et de toute façon ce n'est pas le bon endroit.
2. C'est toi qui gères @Eragon , personne d'autre n'utilise ce système
3. Idem
4. De la doc en quelque sorte, relativement historique. Pas présent en prod.
5. Les imports servent à exposer toutes les parties de l'application en évaluant les décorateurs lors du chargement du fichier racine.
6. Idem
7. C'est un fichier de données servant à initialiser la base de données.
8. Idem
9. Idem
10. Idem
11. Je veux bien y réfléchir mais par principe non, y'a pas le moindre besoin de mettre les avatars en BDD.
12. Je l'ai codé à une époque, mais c'est plus compliqué et ça sert à rien, ces données ne changent jamais. Le besoin n'existe pas vraiment.
13. Je vais m'y intéresser, je ne connais pas encore les blueprints de Flask.
14. Pas très chaud à cause du coût d'intégration et le fait que le panel admin contiendra ultimement surtout des opérations non triviales et donc tout sauf automatiques.
15. Si on faisait du Python, y'aurait pas de quotes. Ici c'est un filtre d'affichage, et le site est en français.
16. Certainement, on s'en fout à la limite, je sais que j'avais regardé mais j'avoue que je sais plus pourquoi on a décidé de le faire. Peut-être une histoire de module en plus.
17. Possible. Dans ce cas y'aurait un fichier YAML puisqu'il faut bien initialiser la bdd.

Bon finalement je suis relativement d'accord, les points non pertinents pour moi étant liés au fait que Fomys ne sait pas comment le code est construit. Ce qui revient à mon point d'origine : c'est pas très pertinent de faire une code review dans ce contexte.

Euh... c'est gentil de sa part, et je vais définitivement regarder individuellement ces remarques. Cela dit c'est pas orthodoxe d'évaluer le travail existant avant d'avoir prouvé ses propres compétences, donc je n'irai pas forcément chercher très loin quand j'estimerai devoir réfuter les remarques. 1. Oui c'est exact, on ne s'en sert pas actuellement, et de toute façon ce n'est pas le bon endroit. 2. C'est toi qui gères @Eragon , personne d'autre n'utilise ce système 3. Idem 4. De la doc en quelque sorte, relativement historique. Pas présent en prod. 5. Les imports servent à exposer toutes les parties de l'application en évaluant les décorateurs lors du chargement du fichier racine. 6. Idem 7. C'est un fichier de données servant à *initialiser* la base de données. 8. Idem 9. Idem 10. Idem 11. Je veux bien y réfléchir mais par principe non, y'a pas le moindre besoin de mettre les avatars en BDD. 12. Je l'ai codé à une époque, mais c'est plus compliqué et ça sert à rien, ces données ne changent jamais. Le besoin n'existe pas vraiment. 13. Je vais m'y intéresser, je ne connais pas encore les blueprints de Flask. 14. Pas très chaud à cause du coût d'intégration et le fait que le panel admin contiendra ultimement surtout des opérations non triviales et donc tout sauf automatiques. 15. Si on faisait du Python, y'aurait pas de quotes. Ici c'est un filtre d'affichage, et le site est en français. 16. Certainement, on s'en fout à la limite, je sais que j'avais regardé mais j'avoue que je sais plus pourquoi on a décidé de le faire. Peut-être une histoire de module en plus. 17. Possible. Dans ce cas y'aurait un fichier YAML puisqu'il faut bien initialiser la bdd. Bon finalement je suis relativement d'accord, les points non pertinents pour moi étant liés au fait que Fomys ne sait pas comment le code est construit. Ce qui revient à mon point d'origine : c'est pas très pertinent de faire une code review dans ce contexte.

Lephenixnoir commented 2019-09-09 13:30:48 +02:00

Owner

Copy Link

Eh ben je vois que pendant que j'ai rédigé ça on est tombés sur la même longueur d'onde avec Darks. Ça fait plaisir à voir !

Tu ne les utilise pas, Lephé utilise virtualenv et moi pipenv x)

Nah, j'utilise plus virtualenv depuis un très long moment.

Eh ben je vois que pendant que j'ai rédigé ça on est tombés sur la même longueur d'onde avec Darks. Ça fait plaisir à voir ! > Tu ne les utilise pas, Lephé utilise virtualenv et moi pipenv x) Nah, j'utilise plus virtualenv depuis un très long moment.

Eragon commented 2019-09-09 13:30:48 +02:00

Author

Member

Copy Link

Dans le cas du n°2 et 3 je vais donc del les fichiers du git et les mettre dans le gitignore il me semblait que l'un d'entre vous utilisait ça quand même

Dans le cas du n°2 et 3 je vais donc del les fichiers du git et les mettre dans le gitignore il me semblait que l'un d'entre vous utilisait ça quand même

Ghost commented 2019-09-09 16:37:20 +02:00

Copy Link

Me voilà Fomys, le redacteur de ce review,

Je suis parfaitement daccord avec vous sur le fait qur je ne connais pas votre fonctionnement, mais un point de vue exterieur ne peut que apporter de nouvelles idees (ou alors de idees que vous avez deja ecarté) donc cest interresant, apres pour ma connaissance de flask je ne lai jamais utilisé sur des projets publics (un jour j'espere) mais je lai bc utilisé pour la creation d'une api (abandonnée depuis, car 0 motiv) (donc moins important que vs) mais jai deja ete confronté à un certain nombre de problemes (photo notamment).

Je vais me repencher ce soir de manière plus avancée (hier soir jai juste fait un ficher apres lautre, des verifications "basique" sur la comprehensibilité du code, et un peut de l'architecture) sur le projet et essayer de resoudre qques issues (ou alors faire autre chose si vs voulez, voire meme partir si ca vous gene).

PS: Si certaines remaques sont sous la forme "il faut" cest juste une erreur, cest a remplacer par "jaurais fait"

Me voilà Fomys, le redacteur de ce review, Je suis parfaitement daccord avec vous sur le fait qur je ne connais pas votre fonctionnement, mais un point de vue exterieur ne peut que apporter de nouvelles idees (ou alors de idees que vous avez deja ecarté) donc cest interresant, apres pour ma connaissance de flask je ne lai jamais utilisé sur des projets publics (un jour j'espere) mais je lai bc utilisé pour la creation d'une api (abandonnée depuis, car 0 motiv) (donc moins important que vs) mais jai deja ete confronté à un certain nombre de problemes (photo notamment). Je vais me repencher ce soir de manière plus avancée (hier soir jai juste fait un ficher apres lautre, des verifications "basique" sur la comprehensibilité du code, et un peut de l'architecture) sur le projet et essayer de resoudre qques issues (ou alors faire autre chose si vs voulez, voire meme partir si ca vous gene). PS: Si certaines remaques sont sous la forme "il faut" cest juste une erreur, cest a remplacer par "jaurais fait"

👍 3

Ghost commented 2019-09-09 19:47:48 +02:00

Copy Link

Pour le point 5, j'ai vérifié la doc et le code de flask login ya pas d'autre moyen, c'est codé avec le cul, certains paramètres sont récup de la config (genre ce qui concerne les cookies) mais pas celui la (sauf si vous décidez d'utiliser gettext ou equivalent).

Pour le point 5, j'ai vérifié la doc et le code de flask login ya pas d'autre moyen, c'est codé avec le cul, certains paramètres sont récup de la config (genre ce qui concerne les cookies) mais pas celui la (sauf si vous décidez d'utiliser gettext ou equivalent).

Darks added the

proposal

label 2019-12-05 00:27:26 +01:00

Darks commented 2020-08-06 00:15:48 +02:00

Owner

Copy Link

Pas mal de trucs ont changé depuis cette review, certaines remarques ont été intégrées d'une manière ou d'une autre.

Pour celles qui resteraient en suspend, j'invite à créer des tickets séparés pour garder la traçabilité de chaque.

Pas mal de trucs ont changé depuis cette review, certaines remarques ont été intégrées d'une manière ou d'une autre. Pour celles qui resteraient en suspend, j'invite à créer des tickets séparés pour garder la traçabilité de chaque.

Darks closed this issue 2020-08-06 00:15:48 +02:00

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

master

preprod

dev

search

landing_page

account/md_editor

debug_toolbar

new_editor

widgets

api/admin/members

Labels

Clear labels   

Core

Required to release v5

  

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

easy

The fix is considered as easy to make

  

enhancement

New feature

  

help wanted

Need some help

  

invalid

Something is wrong

  

performance

Something is slow, not optimized

  

proposal

Feature to be discussed, RFC

  

question

More information is needed

  

security

This issue is a security breach

  

warning

It works, but for how long?

  

wontfix

This won't be fixed

No Label

Core

bug

duplicate

easy

enhancement

help wanted

invalid

performance

proposal

question

security

warning

wontfix

Milestone

Clear milestone

No items

No Milestone

Assignees

Clear assignees

No Assignees

4 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: devs/PCv5#29

No description provided.