-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
move all environment variables to the same place #4192
move all environment variables to the same place #4192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is much cleaner with this PR, should also allow us to re-introduce envfile usage in a follow up PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return path | ||
|
||
|
||
def interpret_env_var_value(value: str, field: dataclasses.Field) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably needs handling for Union as well, see #4205
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not using union for our field types though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(at least not more than just optional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're not using union for our field types though
What about bun_path
? https://github.com/reflex-dev/reflex/blob/main/reflex/config.py#L194
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bun_path
goes through a different code path, and it doesn't use interpret_env_var_value
(maybe it should?), the issue here is we accept str
but we're going to convert it to Path
anyways, so it acts like a nice API but not true "type", i propose changing it to just Path
instead of Union
(which does kick the bucket of figuring out how to do true unions to a later day)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, you are right. I haven't realized that your PR did not touch update_from_env
at all. Maybe I should just base #4205 on your PR and utilize your interpret_env_var_value
to streamline this type parsing logic.
i propose changing it to just Path instead of Union
sounds good!
(which does kick the bucket of figuring out how to do true unions to a later day)
let's raise an exception for unhandled unions to prevent someone introducing a bug by adding a union to env/config
f.e. BUN_PATH is currently broken on main due to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds alright by me, we can merge this for now
* move all environment variables to the same place * reorder things around * move more variables to environment * remove cyclical imports * forgot default value for field * for some reason type hints aren't being interpreted * put the field type *before* not after * make it get * move a bit more * add more fields * move reflex dir * add return * put things somewhere else * add custom error
Fixes #4172