#29 Review du code par Fomys en mp sur Discord

Open
opened 1 month ago by Eragon · 7 comments
Eragon commented 1 month ago

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

image

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 image

Fomys m'a fait une review du code hier soir. Je vais donc poster ce qu'il à dit ici: ![image](/attachments/e5a8cf6e-e734-4f82-a90d-ea7d15415d8e) ```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](/attachments/8ccb2408-8ad7-4e90-a4c6-f2acf6135847) ![image](/attachments/8b905a21-437f-40cf-92fc-9384505ea79e)
Darks commented 1 month ago
Owner

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:

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 1 month ago
Poster

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.

  1. 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 1 month ago
Owner

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 1 month ago
Owner

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 1 month ago
Poster

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
fomys commented 1 month ago

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"
fomys commented 1 month ago

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).
Sign in to join this conversation.
No Milestone
No Assignees
4 Participants
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.