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

CSV configuration variables #105

Merged
merged 15 commits into from
Aug 8, 2022
Merged

CSV configuration variables #105

merged 15 commits into from
Aug 8, 2022

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 3, 2022

closes #62

What this PR does

Adds the following environment variables to allow developers to configure the CSV file. Updates .env.sample and .env.settingstest to reflect new variables. Configures values to be compatible with python setup.py and the pytests. Updates documentation.

Up next: Refactoring the way settings are configured, #109, so that it is easier to write pytests for different configuration scenarios.

Environment variables

  • CSV_NEWLINE=""
  • CSV_DELIMITER=;
  • CSV_QUOTING=3
  • CSV_QUOTECHAR=""

These variables come straight from https://docs.python.org/3/library/csv.html#examples

@angela-tran
Copy link
Member

@machikoyasuda and I learned today that ; is a control operator in bash, so we need to wrap it in quotes when setting it as an environment variable.

https://stackoverflow.com/a/67507121

@machikoyasuda machikoyasuda marked this pull request as ready for review August 5, 2022 18:28
@machikoyasuda machikoyasuda requested a review from a team as a code owner August 5, 2022 18:28
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

This looks great, nice work @machikoyasuda 👍


## Set up database and run tests
Starting the Dev Container will run `bin/start.sh`, which runs `setup.py` and starts the Flask app. The `setup.py` script creates a table, imports and saves users from a JSON or CSV file specified in the .env file from the `IMPORT_FILE_PATH` key. CSV files will require
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for updating these docs! Especially with the part about bin/start.sh, which I missed in #101.

I don't want to block this PR, but I noticed this fragment CSV files will require. Maybe we can finish it out or remove it as a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will take care of this in the next PR.

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.

Make CSV options configurable
3 participants