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

Updated README for getting test environment up #1737

Merged
merged 3 commits into from
Sep 5, 2024
Merged

Conversation

rachel-lawrie
Copy link
Collaborator

@rachel-lawrie rachel-lawrie commented Aug 22, 2024

Fixes #1759

@rachel-lawrie rachel-lawrie marked this pull request as draft August 28, 2024 12:13
Updated README.md to detail Python versions needed and how to set up local testing.
@rachel-lawrie rachel-lawrie marked this pull request as ready for review September 4, 2024 03:29
README.md Outdated
4. Start a server on localhost to see your changes
### 4. Start a server on localhost to see your changes

In api.py, comment out
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can actually remove the CORS instructions, since the CORS errors were occurring due to Apple bluetooth jacking port 5000

README.md Outdated

Return to policyengine-api folder and relevant python environment and run:
```
POLICYENGINE_DB_PASSWORD="" python policyengine_api/worker.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will sound super nitpicky, but we've had issues in the past, so you might want to write something like "PASSWORD_HERE" inside the quotes

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up-to-date, @rachel-lawrie, this was sorely needed. I left a couple minor suggestions, but after that, it'd be great to have this merged!

README.md Outdated

### 6. Testing calculations

To test anything that utilizes Redis or the API's service workers (e.g. anything that requires calculations), you'll also need to complete the following steps:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
To test anything that utilizes Redis or the API's service workers (e.g. anything that requires calculations), you'll also need to complete the following steps:
To test anything that utilizes Redis or the API's service workers (e.g. anything that requires society-wide calculations with the policy calculator), you'll also need to complete the following steps:

Technically, this doesn't affect household calculations

Incorporated Anthony's comments from his review
Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Thanks for this comprehensive workup, @rachel-lawrie! This'll be great to have for onboarding new contributors.

@anth-volk anth-volk merged commit 0bc43d8 into master Sep 5, 2024
4 checks passed
@anth-volk anth-volk deleted the lawrie_readme branch September 5, 2024 18:33
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.

README is out of date
2 participants