-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API de recherche de stops GTFS par bounding box #3517
Conversation
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Outdated
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Outdated
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Outdated
Show resolved
Hide resolved
Co-authored-by: Antoine Augusti <antoine.augusti@transport.data.gouv.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je trouverais utile d'avoir une description (désolé si je l'ai ratée) plus explicite dans la doc contrôleur et/ou dans la PR des "deux modes" et de leur fonctionnement, car à la lecture c'est pas évident à comprendre (sachant que le code de la feature est déjà un peu "spécifique").
Je comprends que tant que la personne utilise la feature comme elle est documenté en opération OpenAPI, on restera sur du non clusteré, par exemple, et que si elle se mettait à passer autre chose, on lui retournerait du clusteré.
Cette dualité gagnerait à être décrite explicitement pour qu'on (nous ou de futurs mainteneurs) ne s'y perde pas.
Je ferai un peu de test en local aussi niveau perf pour voir si je ne vois aucun souci, pour l'instant j'ai juste pris le temps de lire le code.
Pas évident à relire sur ce type de PR (sans critique aucune) dans le sens où le code bouge en bloc et est modifié pour les besoins du changement, je ferai une petite comparaison sur deux répertoires pour ne rien rater.
apps/transport/test/transport_web/controllers/api/gtfs_stops_controller_test.exs
Show resolved
Hide resolved
apps/transport/lib/transport_web/api/controllers/gtfs_stops_controller.ex
Show resolved
Hide resolved
J’ai documenté ça dans un @moduledoc dans le contrôleur, dis-moi si c’est bien @thbar ! |
end | ||
|
||
defp parse_map_params(_params), do: :no_map_params | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdegove ici il aurait été pertinent d'utiliser des doctests, mais on verra plus tard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beau travail @vdegove!
La doc sur le module m'a parue claire aussi, merci.
J'ai vérifié les aspects suivants:
- non régression tant fonctionnelle que performance (sauf erreur)
J'ai apporté quelques changements:
- DRY et amélioration (mise en gras, tout en haut de la description) de l'aspect mise en avant du côté expérimental (voir copie d'écran plus bas)
- Config OpenAPISpex pour désactiver le cache en local (sinon il faut redémarrer à chaque tour et c'est pénible)
- Amélioration de la doc pour documenter ce qui se passe en cas de nombre de points excessif
On gagnerait à ajouter des "doctests" autour des fonctions de parsing, @vdegove je te laisse te chauffer dessus sur une prochaine PR si tu veux, c'est un bon exercice !
Copie d'écran du rendu après modification:
# Uncomment this to avoid having to restart the app for each change in description | ||
# See https://github.com/open-api-spex/open_api_spex#serve-the-spec | ||
# config :open_api_spex, :cache_adapter, OpenApiSpex.Plug.NoneCache | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdegove @AntoineAugusti si vous avez besoin de travailler sur la partie OpenAPI, pensez à décommenter ça que je viens de découvrir car ça fatiguait de redémarrer, c'est très pratique ! (je ne l'ai pas laissé par défaut car pas sûr de l'impact sur le reste en dév, type recompilation lente ou autre etc)
Cette PR reprend le code déjà fait pour la carte des stops GTFS et le transforme en API :
Notes @thbar