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

fix: updated the folksonomy install-md file #233

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

parthshah02
Copy link

What

  • as assigned made the changes to Folksonomy API install guide

@parthshah02 parthshah02 requested a review from a team as a code owner March 1, 2025 04:00
@parthshah02 parthshah02 changed the title updated the folksonomy install-md file fix: A bug fix- updated the folksonomy install-md file Mar 1, 2025
@parthshah02 parthshah02 changed the title fix: A bug fix- updated the folksonomy install-md file updated the folksonomy install-md file Mar 1, 2025
@parthshah02 parthshah02 changed the title updated the folksonomy install-md file fix: updated the folksonomy install-md file Mar 2, 2025
Copy link
Member

@CharlesNepote CharlesNepote left a comment

Choose a reason for hiding this comment

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

Thanks @parthshah02! This is almost done, but not completely. Could you have a look at my comments?


```pip install -r requirements.txt```

# Step 6: Set Up PostgreSQL Database
Copy link
Member

@CharlesNepote CharlesNepote Mar 3, 2025

Choose a reason for hiding this comment

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

Here I think you must explain the two options:

  • either using a local postgre
  • either using ./start_postgres.sh, which launch a ready to use postgre container

The documentation should clearly explain both cases.

@@ -69,15 +95,12 @@ cp local_settings_example.py local_settings.py
python ./db-migration.py
Copy link
Member

Choose a reason for hiding this comment

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

Redundant with step 6 where you already use yoyo apply --database postgresql:///folksonomy?

I would put python ./db-migration.py in a 8th step.

The documentation should make the difference between the things you have to do one time, and the things you have to do every time you want to get back to your dev environment.

Either you can mention in comment the steps which need to be setup only one time. Either you can add a second part at the end of the doc:

# Relaunch dev environment after install

If you have closed your computer and you want to relaunch dev environement, you have to redo some of the previous steps, but not all ones.
[...]

parthshah02 and others added 2 commits March 3, 2025 19:14
Co-authored-by: Charles Nepote <[email protected]>
Co-authored-by: Charles Nepote <[email protected]>
@CharlesNepote
Copy link
Member

CharlesNepote commented Mar 3, 2025

Using numbered steps is a good idea, and you're almost done.

The goal of this document:

  • anyone with few skills should be able to have a working dev environment
  • anyone should also be able to relaunch the environment after having turning off/on her/his computer
  • there are two different options for the database, it should be more clear.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.49%. Comparing base (b9cda65) to head (b474a9a).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
- Coverage   95.06%   94.49%   -0.58%     
==========================================
  Files           5        5              
  Lines         324      363      +39     
==========================================
+ Hits          308      343      +35     
- Misses         16       20       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants