entrouvert
/
chrono
14
1
Fork 0
Code Issues Pull Requests 4 Releases Wiki Activity

api: add 'max_places' argument to API (#89848) #249

Merged
yweber merged 1 commits from wip/89848-add-max_places-param-to-api into main 2024-04-30 14:58:06 +02:00
Conversation 0 Commits 1 Files Changed 4 +86 -12
yweber commented 2024-04-22 10:52:52 +02:00
Owner
Copy Link
No description provided.
yweber added 1 commit 2024-04-22 10:52:53 +02:00
gitea/chrono/pipeline/head There was a failure building this commit Details
bc9e9e79cc api: add 'max_places' argument to API (#89848)
yweber force-pushed wip/89848-add-max_places-param-to-api from bc9e9e79cc to ece23b6d89 2024-04-22 10:59:08 +02:00 Compare
yweber changed title from WIP: api: add 'max_places' argument to API (#89848) to api: add 'max_places' argument to API (#89848) 2024-04-22 11:20:07 +02:00
vdeniaud requested changes 2024-04-30 10:44:11 +02:00
Dismissed
vdeniaud left a comment
Owner
Copy Link

Une remarque de fond qui ne devrait pas occasionner beaucoup de changements, le reste m'a l'air parfait !

Une remarque de fond qui ne devrait pas occasionner beaucoup de changements, le reste m'a l'air parfait !
chrono/api/serializers.py Outdated
@ -467,6 +467,7 @@ class DateRangeSerializer(DateRangeMixin, serializers.Serializer):
class DatetimesSerializer(DateRangeSerializer):
min_places = serializers.IntegerField(min_value=1, default=1)
max_places = serializers.IntegerField(min_value=0, default=0)
vdeniaud commented 2024-04-30 10:38:37 +02:00
Owner
Copy Link

Ça ne me semble pas utile de permettre max_places=0. En tout cas si c'est possible je m'attends à ce que ça renvoie tous les événements désactivés, mais ce n'est pas le cas car plus bas if 0 < max_places donc 0 est en fait une valeur spéciale qui "désactive" le filtre.

Pour moi ce serait plus clair d'avoir ici

max_places = serializers.IntegerField(min_value=1)

et de gérer une valeur None de max_places ailleurs genre

    if max_places and max_places <= event.remaining_places:
Ça ne me semble pas utile de permettre max_places=0. En tout cas si c'est possible je m'attends à ce que ça renvoie tous les événements désactivés, mais ce n'est pas le cas car plus bas ` if 0 < max_places` donc 0 est en fait une valeur spéciale qui "désactive" le filtre. Pour moi ce serait plus clair d'avoir ici ``` max_places = serializers.IntegerField(min_value=1) ``` et de gérer une valeur None de max_places ailleurs genre ``` if max_places and max_places <= event.remaining_places: ```
yweber commented 2024-04-30 14:07:45 +02:00
Author
Owner
Copy Link

Bien vu, merci !

Par contre j'ai préféré passé explicitement default=None la doc expliquant que "If the key is not present it will simply not be included in the output representation."

Comme ça on a pas a tester la présence de la clé et c'est cohérent avec les valeurs par défauts des fonctions prenant max_places en argument.

Mais j'ai peut être mal saisie les tenants et aboutissants, s'il vaut mieux que je ne passe pas de default je fais la modif :)

Bien vu, merci ! Par contre j'ai préféré passé explicitement `default=None` la doc expliquant que "If the key is not present it will simply not be included in the output representation." Comme ça on a pas a tester la présence de la clé et c'est cohérent avec les valeurs par défauts des fonctions prenant max_places en argument. Mais j'ai peut être mal saisie les tenants et aboutissants, s'il vaut mieux que je ne passe pas de default je fais la modif :)
vdeniaud commented 2024-04-30 14:42:04 +02:00
Owner
Copy Link

Oui très bien si ça évite des .get() partout c'est bon à prendre ! (par contre c'est mieux sans is not None (car outre le côté moins verbeux, c'est classiquement utilisé quand on doit différencier None de False ou de 0, or ce n'est pas le cas ici, donc ça fait se poser des questions pour rien :) ))

Oui très bien si ça évite des `.get()` partout c'est bon à prendre ! (par contre c'est mieux sans `is not None` (car outre le côté moins verbeux, c'est classiquement utilisé quand on doit différencier None de False ou de 0, or ce n'est pas le cas ici, donc ça fait se poser des questions pour rien :) ))
yweber commented 2024-04-30 14:51:30 +02:00
Author
Owner
Copy Link

par contre c'est mieux sans is not None

Ah ben oui, bien vu ! C'est modifié

> par contre c'est mieux sans `is not None` Ah ben oui, bien vu ! C'est modifié
yweber force-pushed wip/89848-add-max_places-param-to-api from ece23b6d89 to 45f68555ac 2024-04-30 14:04:01 +02:00 Compare
yweber requested review from vdeniaud 2024-04-30 14:08:05 +02:00
yweber force-pushed wip/89848-add-max_places-param-to-api from 45f68555ac to fe0ea57056 2024-04-30 14:21:36 +02:00 Compare
vdeniaud approved these changes 2024-04-30 14:42:12 +02:00
yweber force-pushed wip/89848-add-max_places-param-to-api from fe0ea57056 to 5159914c9f 2024-04-30 14:49:55 +02:00 Compare
yweber force-pushed wip/89848-add-max_places-param-to-api from 5159914c9f to 6c2c412cfc 2024-04-30 14:50:54 +02:00 Compare
yweber merged commit 6c2c412cfc into main 2024-04-30 14:58:06 +02:00
yweber deleted branch wip/89848-add-max_places-param-to-api 2024-04-30 14:58:06 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
vdeniaud
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
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: entrouvert/chrono#249
No description provided.
Delete Branch "wip/89848-add-max_places-param-to-api"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?