Skip to content

More env var cleanup #4248

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

Merged
merged 36 commits into from
Nov 5, 2024
Merged

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Oct 26, 2024

closes #4172

benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Oct 28, 2024
@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Oct 28, 2024

todo: add a flag to disable Path.exists validation

edit: i created #4253

masenf pushed a commit that referenced this pull request Oct 28, 2024
* port enum env var support from #4248

* add some tests for interpret env var functions
benedikt-bartscher added a commit to benedikt-bartscher/reflex that referenced this pull request Oct 28, 2024
@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review October 28, 2024 22:03
masenf pushed a commit that referenced this pull request Oct 29, 2024
* port enum env var support from #4248

* add some tests for interpret env var functions
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

overall i like the descriptor implementation

i'm not too sure about .get being a property though, to me that kind of looks weird. did you consider having it as a normal method like .set(...)

@adhami3310 wondering if you have any input here as well

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Oct 31, 2024

overall i like the descriptor implementation

Thanks, I wanted to try binding the descriptors to rx.Config in a followup.

i'm not too sure about .get being a property though, to me that kind of looks weird. did you consider having it as a normal method like .set(...)

Yeah, I was thinking about making it a normal method… This would also allow us to add f.e. a cache arg to get as well.
If @adhami3310 agrees, I will refactor it.

@adhami3310
Copy link
Member

If @adhami3310 agrees i will refactor it.

yea that would be great

@benedikt-bartscher
Copy link
Contributor Author

Refactor done ✔️

@benedikt-bartscher
Copy link
Contributor Author

Can we get a review on this one? It is blocking #3728 and #4303

@masenf masenf merged commit 4a6c16e into reflex-dev:main Nov 5, 2024
29 checks passed
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.

Unify env vars
3 participants