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

Remove configs.py #256

Merged
merged 6 commits into from
Aug 15, 2023
Merged

Remove configs.py #256

merged 6 commits into from
Aug 15, 2023

Conversation

haneslinger
Copy link
Collaborator

We dont need this anymore, we've got .env

Copy link
Member

@MatthewSteen MatthewSteen left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion and needs a test fix.

docs/reference/cli_tool.md Outdated Show resolved Hide resolved
Co-authored-by: Matt Steen <MatthewSteen@users.noreply.github.com>
@haneslinger haneslinger requested a review from MatthewSteen May 26, 2023 16:52
Copy link
Member

@TShapinsky TShapinsky left a comment

Choose a reason for hiding this comment

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

Looks good, we could also consider incorporating or linking to python-dotenv which automatically brings in a .env file as environment variables https://pypi.org/project/python-dotenv/

buildingmotif/api/app.py Show resolved Hide resolved
Copy link
Member

@TShapinsky TShapinsky left a comment

Choose a reason for hiding this comment

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

Looks good to me once tests pass and it's merged

@TShapinsky
Copy link
Member

  1. Provide a couple of examples for loading env variables on windows/linux/mac
  2. Link to python-dotenv as a possible way to load from .env cleanly

@MatthewSteen MatthewSteen self-requested a review June 20, 2023 19:21
@haneslinger
Copy link
Collaborator Author

@MatthewSteen re-pinging this.

@MatthewSteen
Copy link
Member

MatthewSteen commented Aug 14, 2023

@haneslinger I think this just needs some brief documentation about the BASH requirement to run on Windows. See my comments and suggestions above, specifically #256 (comment).

Looking at this a bit more, I think it just requires a Windows user to use bash, which is included with Git. So maybe just keep as is and note that in the docs, which we could revisit later. The python-dotenv solution could be clean but I generally don't like adding dependencies if they can be avoided.

@haneslinger
Copy link
Collaborator Author

@MatthewSteen Oh, yes, thank you. Would you be able to add that? Do you want it in this PR?

@MatthewSteen
Copy link
Member

@haneslinger I added a note for windows users, which I think is sufficient for now. Take a look and merge when ready.

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.

4 participants