Skip to content
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

Refacto get dataset betagouv #77

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

fufeck
Copy link
Contributor

@fufeck fufeck commented Sep 11, 2023

Context

Fonctionnalité

  • Prise en compte de la pagination pour le pas avoir a augmenter le page_size dans quelque mois.
  • Suppression de la requête qui récupère l'organisation de chaque dataset, car cette dernière est présente dans la requète de base
  • Url de la route api de beta.gouv mise en variable d'environnement

Doc API datasets

https://guides.data.gouv.fr/publier-des-donnees/guide-data.gouv.fr/api/reference/datasets

@fufeck fufeck requested a review from MaGOs92 September 11, 2023 10:17
@fufeck fufeck changed the title Fufeck refacto get dataset betagouv Refacto get dataset betagouv Sep 12, 2023
MaGOs92
MaGOs92 previously approved these changes Sep 14, 2023
Copy link
Contributor

@MaGOs92 MaGOs92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

.env.sample Outdated Show resolved Hide resolved
@MaGOs92
Copy link
Contributor

MaGOs92 commented Sep 14, 2023

Mince j'ai des soucis pour tester, un bug bizarre de connexion à Mongo... Je pense que c'est encore un problème lié à la lib "gdal-async" qu'on ne peut pas utiliser en node >= 16.
En regardant un peu, je me suis aperçu que cette lib est utilisé par le reader shp qui n'est utilisé que pour ce dataset : https://opendata.arcgis.com/datasets/ce237a8f47bc450d9abd6bfa84a44f74_3.zip (qui pointe maintenant vers une erreur 404).
Je pense donc qu'on devrait pouvoir se débarrasser de cette dépendance, tu en penses quoi?
Plus globalement, on pourrait aussi voir si les autres datasets dans les configs sont tjs valides.
A mon avis, le plus solide ça serait de moissonner les données uniquement depuis data.gouv

lib/sources/datagouv.js Outdated Show resolved Hide resolved
@MaGOs92
Copy link
Contributor

MaGOs92 commented Sep 15, 2023

Bon bizarrement aujourd'hui ça fonctionne, sans doute un souci de conf de mon côté hier.
Tests LGTM, j'ai fait la PR d'authentification du moissonnage sur cette PR

@fufeck
Copy link
Contributor Author

fufeck commented Sep 25, 2023

Mince j'ai des soucis pour tester, un bug bizarre de connexion à Mongo... Je pense que c'est encore un problème lié à la lib "gdal-async" qu'on ne peut pas utiliser en node >= 16. En regardant un peu, je me suis aperçu que cette lib est utilisé par le reader shp qui n'est utilisé que pour ce dataset : https://opendata.arcgis.com/datasets/ce237a8f47bc450d9abd6bfa84a44f74_3.zip (qui pointe maintenant vers une erreur 404). Je pense donc qu'on devrait pouvoir se débarrasser de cette dépendance, tu en penses quoi? Plus globalement, on pourrait aussi voir si les autres datasets dans les configs sont tjs valides. A mon avis, le plus solide ça serait de moissonner les données uniquement depuis data.gouv

A regarder en effet, je vais faire un petit check a la mano ;)
Et pour l'aglo de Chalons, j'ai demandé au reste de l'équipe

@fufeck fufeck requested a review from MaGOs92 September 25, 2023 13:02
MaGOs92
MaGOs92 previously approved these changes Sep 26, 2023
lib/sources/datagouv.js Outdated Show resolved Hide resolved
Copy link
Contributor

@MaGOs92 MaGOs92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fufeck fufeck merged commit 2dcaaea into master Oct 2, 2023
2 checks passed
@fufeck fufeck deleted the fufeck_refacto_get-dataset-betagouv branch October 2, 2023 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants