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

Add interpolate option #419

Merged

Conversation

David-Wobrock
Copy link
Contributor

The argument is already documented but not implemented yet.
Fixes #415

@WTK
Copy link

WTK commented Dec 1, 2022

I believe this doesn't solve the problem I see with the code: when using interpolation and cast together you'll get unexpected error. Minimal example:

# .env file contents:
# BAR=3.2
# FOO=$BAR

import environ
env = environ.Env(interpolate=True)
env.str('FOO') # -- outputs "3.2" as expected
env.float('FOO') # <- errores out, as it tries to make float out of not expanded string literal "BAR"

@David-Wobrock
Copy link
Contributor Author

Hey @WTK

Thanks for the comment, I think it would be worth creating another PR to fix the issue you are mentioning.
The hereby PR is adding the logic to deactivate the interpolate option, whereas, from what I'm understanding, you are mentioning that interpolation doesn't work properly.

@sergeyklay sergeyklay added the enhancement New feature or request label Mar 1, 2023
@ThalusA
Copy link

ThalusA commented Jul 10, 2023

@sergeyklay I see you are now the main contributor with push access to this repository, would you mind merging this simple PR ? It just makes interpolate works like it is supposed to be. It doesn't create new errors or anything like that.

@David-Wobrock
Copy link
Contributor Author

Rebased to fix the conflict.

@coveralls
Copy link

coveralls commented Jul 14, 2023

Coverage Status

coverage: 92.5% (+0.2%) from 92.255% when pulling 8c1b1c1 on David-Wobrock:feat/add-interpolate-option into 2750dd5 on joke2k:develop.

Copy link
Collaborator

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

@David-Wobrock I've been looking through your pull request and in principle, everything looks good. However, there seem to be a few builds that failed, notably the code style checks and linters. Could you please make the necessary adjustments? It would greatly assist in maintaining our codebase's quality and readability. Also, I would appreciate it if you could add an entry in the changelog about these changes as well as update corresponding article https://github.com/joke2k/django-environ/blob/main/docs/tips.rst#proxy-value. This would save me some time and ensure that we keep our documentation up to date.

environ/environ.py Outdated Show resolved Hide resolved
@sergeyklay sergeyklay self-assigned this Jul 14, 2023
The argument is already documented but not implemented yet.
Fixes joke2k#415
@David-Wobrock
Copy link
Contributor Author

Hi @sergeyklay

Thanks for your input 👍

However, there seem to be a few builds that failed

I adapted the code ✔️ We should be good now (linting passes locally).
I forced-push, to keep one lean commit, but it seems that the PR requires new approval for running the CI 😕

add an entry in the changelog about these changes

Done ✔️

update corresponding article https://github.com/joke2k/django-environ/blob/main/docs/tips.rst#proxy-value

From my understanding, everything is already documented in the tips section.
That was one of the major reasons of the PR: the logic was documented, but not implemented 😄

@sergeyklay sergeyklay merged commit 797101b into joke2k:develop Jul 17, 2023
@sergeyklay
Copy link
Collaborator

Thank you!

@David-Wobrock David-Wobrock deleted the feat/add-interpolate-option branch July 17, 2023 15:52
@stegayet stegayet mentioned this pull request Aug 14, 2023
@sergeyklay
Copy link
Collaborator

sergeyklay commented Aug 30, 2023

@David-Wobrock Thank you for your contribution to the project. Unfortunately, after implementing your interpolation solution, we've encountered issues from our users. Due to this, we've decided to revert your changes and release a patch version.

This is not a reflection on the quality of your code or idea; it's just that it has caused some complications in this particular context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants