Produits Entr'ouvert » EOPayment

Mouais, en plus d'être pas ouf niveau lisibilité cette approche perd la signature commune de la méthode imposée par la classe Payment, je pense qu'il ne faut pas faire comme ça.

Plutôt implémenter la méthode dans PaymentCommon histoire que tous les backends l'aient, et lui faire retourner None ou une constante par défaut.

Valentin Deniaud a écrit :

Plutôt implémenter la méthode dans PaymentCommon histoire que tous les backends l'aient, et lui faire retourner None ou une constante par défaut.

Bon ben je vais rajouter un flag alors, ça me paraissait vraiment plus simple de faire comme cela, on fait ça partout sans avoir de signature commune à partager, ça n'a choqué personne (wcs.fields.Field.store_display_value, etc...).

Benjamin Dauvergne a écrit :

on fait ça partout sans avoir de signature commune à partager

Ce qui est spécifique ici ça me semble être que Payment est la classe « publique » de eopayment, dont le rôle est de manipuler les backends « privés ». Et donc c'est important que les trucs publics aient une signature fixe.

Valentin Deniaud a écrit :

pas ouf niveau lisibilité

Et je me rends compte que ce qui rend cette histoire cryptique c'est dans common.py

199     payment_status = None

Tu peux peut-être faire sauter ça, en remplaçant par un truc normal genre

def payment_status(self, transaction_id, **kwargs):
    raise NotImplementedError

et retirer le raise de la méthode dans __init__.py ?

Ça empêche naturellement le raccourci que prenait le premier patch, et dans le deuxième patch has_payment_status deviendrait un truc qui rattrape l'exception. Mais ça m'aiderait probablement de voir ou tout ça va être utilisé, il y a un ticket côté lingo ?

Valentin Deniaud a écrit :

Ça empêche naturellement le raccourci que prenait le premier patch, et dans le deuxième patch has_payment_status deviendrait un truc qui rattrape l'exception.

Tu me demandes de faire ce que ce ticket essaie d'éviter depuis le début, appeler la méthode pour savoir si elle existe.

Mais ça m'aiderait probablement de voir ou tout ça va être utilisé, il y a un ticket côté lingo ?

#49149 le code n'y est pas encore, je dois le réécrire pour ces changements.

Benjamin Dauvergne a écrit :

Valentin Deniaud a écrit :

Ça empêche naturellement le raccourci que prenait le premier patch, et dans le deuxième patch has_payment_status deviendrait un truc qui rattrape l'exception.

Tu me demandes de faire ce que ce ticket essaie d'éviter depuis le début, appeler la méthode pour savoir si elle existe.

J'ai pas l'impression, je suis d'accord sur le principe du patch qui est pour moi « c'est nul d'avoir une lib qui lève des NotImplementedError qu'il faut ensuite gérer ». Mais à partir du moment où c'est géré en interne ça me paraissait OK (et davantage dans les clous que d'avoir une méthode qui des fois est une méthode, des fois est None).

Mais OK pour ne pas faire comme ça, je reprends, dans le patch :

244        return bool(getattr(self.backend, 'payment_status', False))

C'est bizarre comme ligne, sans bizarrerie ça devrait être

244        return hasattr(self.backend, 'payment_status')

Et on en revient à virer ce payment_status = None, en ne mettant rien à la place (avec petite adaptation à Payment.payment_status).