Produits Entr'ouvert » Authentic 2

C'est cool ça va dans le sens d'une simplification. Par contre c'est quand même bien massif, la relecture est difficile, je serai pour attendre le prochain cycle et pousser ça un vendredi.

Ci dessous deux remarques formulées avant que mon cerveau ne s'embourbe.

Il y a une nécessité à ce que set_service puisse nettoyer la session (session.pop('sevice_type', None) ...) ?
En tous cas à la lecture je ne vois pas les moments où on passerait dans ce chemin de code, et si je les loupe, ça me parait être une bonne raison de passer par une autre méthode genre clean_service qui faciliterait la compréhension.

Tout à fait mineur, return getattr(request, '_service', None) overkill, tu es sûr d'avoir une attribut _service.

Emmanuel Cazenave a écrit :

Il y a une nécessité à ce que set_service puisse nettoyer la session (session.pop('sevice_type', None) ...) ?

Non c'est vrai, mais je pense que je devrait le faire dans set_home_url() si on ne trouve pas de service associé à l'URL, revenir dans un état neutre me paraît la bonne pratique. Je n'ai pas écrit le code dans l'ordre le plus logique pendant que je le faisais et donc j'ai du oublier de le faire.

En tous cas à la lecture je ne vois pas les moments où on passerait dans ce chemin de code, et si je les loupe, ça me parait être une bonne raison de passer par une autre méthode genre clean_service qui faciliterait la compréhension.

Éventuellement, mais toujours dans set_home_url() je pense.

Tout à fait mineur, return getattr(request, '_service', None) overkill, tu es sûr d'avoir une attribut _service.

Yep.

Paul Marillonnet a écrit :

• Dans auth_fc :
  Un espace en trop dans encode_state entre la next_url et la signature, on va se retrouver avec une chaîne vide dans le split ensuite.
  [...]
  D’ailleurs dans decode_state j’ai l’impression que la vérification du nombre d’éléments dans le payload n’est plus cruciale, je pense qu’on peut virer le try … except  par simplement :
  [...]
  (ou du moins si on veut s’assurer qu’il y a au moins trois éléments [...])

Ok, fait avec *dummy.

• Du détail, et par simple curiosité, dans authentic2.utils.misc.select_next_url, je comprends pas pourquoi on instancie un EMPTY = object() et faire la distinction entre None et cet objet ?

Au cas où l'appelant voudrait utiliser None comme valeur par défaut et pas une des valeurs pré-définies ailleurs (home_url, etc...).

Est-ce qu’on ne peut pas simplement faire la même distinction entre None et '', genre
[...]
et auquel cas dans set_home_url avec on appellerait select_next_url avec default='' ?

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

• Pour l’usage de set_service j’ai fait quelques git-grep ici et là, ça m’a l’air bon.
  Il y aurait aussi des modifs pertinentes dans les modules tiers, là à vue de nez je vois par exemple authentic2-cut où on place le slug de service dans la session (dans CUTMiddleware), pour l’utiliser notamment pour les entrées de journal. Je pense qu’on peut faire un ticket pour virer ça, on n’a plus besoin de ce slug dans la session dorénavant.

Oui c'est un peu le but, je vais ouvrir le ticket.

Benjamin Dauvergne a écrit :

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

Ok, je ne connaissais pas.

Oui c'est un peu le but, je vais ouvrir le ticket.

D’ac.

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

Ok, je ne connaissais pas.

Oui c'est un peu le but, je vais ouvrir le ticket.

D’ac.

#61508.

Automatic expiration