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

📝 [Documentation] Add command line examples and templates for importing data #4290

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

bruhnild
Copy link
Contributor

@bruhnild bruhnild commented Sep 3, 2024

Description

Related Issue

Checklist

  • I have followed the guidelines in our Contributing document
  • My code respects the Definition of done available in the Development section of the documentation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes
  • I added an entry in the changelog file
  • My commits are all using prefix convention (emoji + tag name) and references associated issues
  • I added a label to the PR corresponding to the perimeter of my contribution
  • The title of my PR mentionned the issue associated

@bruhnild bruhnild requested a review from a team September 3, 2024 16:42
@bruhnild bruhnild self-assigned this Sep 3, 2024
@bruhnild
Copy link
Contributor Author

bruhnild commented Sep 3, 2024

@babastienne
Copy link
Member

Merci @bruhnild pour cette PR.

Une petite erreur dans la commande docker sur l'import des tronçons. Je vais faire une review pour corriger.

Sinon ça me semble très bien, plus complet et plus clair ! 🙏

Quelques petits points qui me viennent à l'esprit quand je parcours cette page :

  • Section "Import Dive" : Comme c'est un module déprécié je pense qu'il faudrait soit retirer complètement cette partie soit à minima mettre un message en mode "warning" qui indique que ce module n'est plus recommandé et va bientôt disparaitre
  • Est-ce utile de toujours distinguer la commande avec docker ? Pour moi on a déjà expliqué dans cette section comment convertir une commande debian en docker. Du coup je me demande si c'est une bonne idée ? D'un côté c'est plus clair pour les utilisateurs mais sont-ils nombreux à utiliser l'installation docker ? Et surtout est-ce qu'on pensera à mettre à jour la doc partout si demain la commande docker évolue ? Je n'ai pas d'avis tranché sur ce point, on peut en discuter, à voir l'avis des autres en daily ?


.. code-block:: bash

sudo geotrek loadpaths \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sudo geotrek loadpaths \
docker compose run --rm web ./manage.py loadpaths \

@camillemonchicourt
Copy link
Member

Oui pour moi il vaut mieux enlever les parties liées au module Plongée.

Et en effet, je privilégierai d'indiquer que les commandes classiques et ceux en Docker savent comment les adapter avec la doc Docker qui le mentionne.

@bruhnild
Copy link
Contributor Author

bruhnild commented Sep 4, 2024

Merci pour vos retours.

Je suis en accord avec la proposition de retirer les éléments liés au module Plongée, et j'ai effectué les modifications nécessaires.
Je suis également d'accord pour supprimer les commandes d'import pour Docker à chaque section de type d'objets, car ça duplique l'information. Cependant, serait-il possible de conserver un exemple, comme celui des tronçons qui apparaît en premier sur la page ?

@babastienne
Copy link
Member

@bruhnild oui pourquoi pas laisser un exemple dans un encart dédié si tu penses que c'est mieux et plus clair comme ça

@bruhnild bruhnild merged commit 34e82c4 into master Sep 4, 2024
1 check passed
@bruhnild bruhnild deleted the mfu-improve-import-data-in-doc branch September 4, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants