Open David-Wobrock opened 1 year ago
Thanks for the reviews, commits and comments 🙏
415 suggested to disable interpolation by default. Would this not be safer to avoid impacting existing users?
We can do that for sure, but that would be a breaking change of the default logic :) Perhaps it can be done in another PR, since the changes would be mainly unrelated.
The hereby patch is implementing the fact that interpolation can be deactivated basically - so there should be no change to the current default behaviour. If we boil down the code change, we go from something like this:
def fn():
if some_condition():
do_stuff()
to:
def fn(should_do_stuff=True):
if should_do_stuff and some_condition():
do_stuff()
meaning that people currently calling fn()
, it will keep working.
⚠️ the changing behaviour is if people are currently calling Env(interpolate=False)
, which, as the time of writing, has the same behaviour as Env(interpolate=True)
, even though it is already documented as if it worked.
Perhaps this is something we can explicit in the changelog.
What do you think? :)
Thanks for the explanation! I agree that making it explicit in the changelog would be a good thing.
I pushed more documentation in the CHANGELOG @mschoettle Feel free to rephrase :)
Looks good to me!
@David-Wobrock I've merged https://github.com/joke2k/django-environ/pull/500 to fix https://github.com/joke2k/django-environ/issues/499. And now this branch has conflicts that must be resolved. Could you please rebase
Thanks for the review @sergeyklay I rebased and pushed an additional commit on top of the branch :)
Is there an end goal of all the changes with the interpolation/variable expansion described anywhere? I'm trying to figure out what you want to achieve, based on the recent pull requests, erratic releases, and yanking released versions from PyPI.
The way I see it:
VAR_A=$VAR_B
.XXX$VAR_A:YYY$VAR_B
.interpolate
parameter has been added to the Env.__init__()
(v0.11.0) because the variable expansion caused issues, a it was not backward compatible.interpolate
parameter and the variable expansion mechanism have been yanked from PyPI (v0.11.1 and v0.11.2 respectively).So, in terms of the interpolation / variable expansion, we are back in v0.10.0.
In my opinion, bringing back the interpolate
parameter in this PR is a mistake.
interpolate
as "interpolation" suggests that it enables a fully-fledged string interpolation, like the one introduced as "expansion mechanism", which has been since reverted.Env.__init__()
is not backward compatible, as it takes the **scheme
keyword arguments. Introducing any parameter there has a chance to collide with user-defined environment variables, leading to unexpected behavior, especially for long-term users.What I believe should be done (already working on it, I can make a PR once done):
aliasing
, use_proxies
, or similar class variable to Env
, make it work like the interpolate
, and let it be set just like any other already existing flag (smart_cast
, escape_proxy
, prefix
). Default should be True
, as this feature has been available for a very long time.Env.interpolation
flag is set to True
(default: False
).I put the "(Maybe)" in the first point, as this parameter has been added as a workaround for the breaking changes introduced with the variable expansion. Since the feature that made the parameter necessary is reverted, I believe there is no reason to add it back.
But if you really want to do it, at least both mechanisms should be kept alongside each other to ensure backward compatibility. Simply replacing the aliasing/proxying mechanism with a proper interpolation will result in breaking changes in projects where anything resembling a $-prefixed variable is used in the middle of any any value.
EDIT: Added my proposal in #510
Hey @potasiak
Thanks for your thorough message.
- Originally, or at least since 2013, there was a simple mechanism for aliasing/proxying variables that allowed for setting a variable value to another variable with
VAR_A=$VAR_B
.
To be honest, I haven't dug into the past about this.
Is there an end goal of all the changes with the interpolation/variable expansion described anywhere? I'm trying to figure out what you want to achieve, based on the recent pull requests, erratic releases, and yanking released versions from PyPI.
My reasoning is fairly straightforward on this patch:
django-environ
is showing a parameter that does not exist, so which does not have the expected behaviour: https://django-environ.readthedocs.io/en/latest/tips.html?highlight=interpolate#proxy-value.$
to remain exactly the same. Thus a way, to disable "interpolation". Perhaps there is already a way to achieve that, that I'm haven't found in the doc's and code.
- Adding any parameter to the
Env.__init__()
is not backward compatible, as it takes the**scheme
keyword arguments. Introducing any parameter there has a chance to collide with user-defined environment variables, leading to unexpected behavior, especially for long-term users.
Arguably, the parameter is documented, thus not entirely unexpected behaviour. And breaking backwards compat' shouldn't really an issue. One could release a major version and explain and document the change properly.
So, in terms of the interpolation / variable expansion, we are back in v0.10.0.
In my opinion, bringing back the
interpolate
parameter in this PR is a mistake.
It should not be called
interpolate
as "interpolation" suggests that it enables a fully-fledged string interpolation, like the one introduced as "expansion mechanism", which has been since reverted.Since we're back to v0.10.0 (regarding this feature), it gives us a chance to do it again in a more thought-through way.
Fair point. I admit that I made the minimal fix to make my use case work, and to match the code to the documentation. I'm completely open to having a broader solution, that fits more use cases and reworks the lib more in-depth. However, I won't have the bandwidth to contribute to that, I'm sorry.
It's mainly that the project is perhaps lacking maintenance (and no blame for that 🤗). So the "minimal" patch here, even if largely sub-optimal, seemed to me more likely to be accepted and released than a larger undertaking.
I hope that makes sense 🙂
Hello @potasiak @mschoettle @sergeyklay
Any chance we can move forward on this? :)
I'm still against the change in its current state but also decided to switch to https://github.com/sloria/environs in the meantime as it does what I need.
I have also created #510 to show how I believe it should be done.
Regardless, since I've switched to another package, I no longer have vested interest in how this issue will be resolved, so please proceed as you see fit.
Understood. Thanks for submitting the patch @potasiak.
The approaches are similar, except that yours goes a step further into what interpolation
in the context of this packages means and intends to achieve in terms of behaviour.
I'm fine with whatever approach, as the hereby patch is the minimal change that I require (to re-use your phrasing) for the usage of this package :)
Second try, after https://github.com/joke2k/django-environ/pull/419 was reverted here https://github.com/joke2k/django-environ/pull/486.
The reason of the revert was these two issues that arise following the release of a new version: https://github.com/joke2k/django-environ/issues/485 and https://github.com/joke2k/django-environ/issues/490 - however, it seems that these were related to https://github.com/joke2k/django-environ/pull/468 instead.
This time, the PR adds unit tests that prove that it is not breaking existing behaviour.