-
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
Merged
adhami3310
merged 14 commits into
main
from
move-all-environment-variables-to-the-same-place
Oct 21, 2024
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0aeb20f
move all environment variables to the same place
adhami3310 5dffc77
reorder things around
adhami3310 11da6ec
move more variables to environment
adhami3310 3893ccf
remove cyclical imports
adhami3310 d680edf
forgot default value for field
adhami3310 3736844
for some reason type hints aren't being interpreted
adhami3310 e38a85a
put the field type *before* not after
adhami3310 0308912
make it get
adhami3310 a9b7c28
move a bit more
adhami3310 b7fdb1d
add more fields
adhami3310 43a89a4
move reflex dir
adhami3310 4d2a85b
add return
adhami3310 7103341
put things somewhere else
adhami3310 fe0a791
add custom error
adhami3310 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about
bun_path
? https://github.com/reflex-dev/reflex/blob/main/reflex/config.py#L194There 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 useinterpret_env_var_value
(maybe it should?), the issue here is we acceptstr
but we're going to convert it toPath
anyways, so it acts like a nice API but not true "type", i propose changing it to justPath
instead ofUnion
(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 yourinterpret_env_var_value
to streamline this type parsing logic.sounds good!
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