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

adding an env example file #133

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

lavishsaluja
Copy link
Collaborator

No description provided.

@CharlieJCJ
Copy link
Contributor

CharlieJCJ commented Nov 18, 2024

TODO here

  • update __main__.py for curator-viewer to take environment variables if these exist (but I think perhaps the CLI defaults here are sufficient as well?, I think over configuring in environment variables may cause unnecessary confusions)
  • update README for cp .env.example .env operation and how to use .env

.env.example Outdated Show resolved Hide resolved
@lavishsaluja
Copy link
Collaborator Author

  1. updating main.py task -> yes CLI variables are sufficient, should not over engineer this, removed the viewer config variables
  2. for now skipping adding this on README as a lot more people will be visiting the repo for documentation of package instead of development so should avoid all noise for them. what do you think?

@CharlieJCJ
Copy link
Contributor

CharlieJCJ commented Nov 18, 2024

for now skipping adding this on README as a lot more people will be visiting the repo for documentation of package instead of development so should avoid all noise for them. what do you think?

I think should place it somewhere on README to reduce onboarding frictions for less experienced developers and have a few words for its purpose can help a lot when we introduce new things that can be configurable.

README.md Show resolved Hide resolved
@CharlieJCJ CharlieJCJ changed the base branch from main to dev November 18, 2024 23:36
@lavishsaluja
Copy link
Collaborator Author

@CharlieJCJ just made the change of changing collapsible, thanks for the input. it looks better now.

@CharlieJCJ
Copy link
Contributor

CharlieJCJ commented Nov 19, 2024

Thanks!

We can merge this after #140 merge, and include part of 0.1.10, since there are some frontend path changes need to address if we introduce the environment variable, and additional testings. In the meantime, can you resolve merge conflict to dev in README.md, thanks

@CharlieJCJ
Copy link
Contributor

CharlieJCJ commented Nov 21, 2024

A question I'm thinking of is how the README would structure after introducing .env because the current workflow is to do pip install, and no where in prompter and viewer do load_env. Another thing is if user chooses to do the .env thing they would git clone the repo first and the prompter and viewer needs to know where this .env is, which users would poetry install, etc, but then poetry isn't been mentioned at all in the README. Trying to think what's the best way to do here. cc @madiator

@lavishsaluja
Copy link
Collaborator Author

fair @CharlieJCJ - I was just thinking of adding an env for people who want to contribute but I guess best to just totally skip this for now - most people will just use with pip and won't need development side information in README

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.

2 participants