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 #495

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

David-Wobrock
Copy link
Contributor

Second try, after #419 was reverted here #486.

The reason of the revert was these two issues that arise following the release of a new version: #485 and #490 - however, it seems that these were related to #468 instead.

This time, the PR adds unit tests that prove that it is not breaking existing behaviour.

@David-Wobrock David-Wobrock force-pushed the feat/add-interpolate-option-with-more-tests branch 3 times, most recently from 4b4d6a8 to 5e6d49c Compare September 5, 2023 14:21
@David-Wobrock David-Wobrock marked this pull request as ready for review September 5, 2023 14:50
CHANGELOG.rst Outdated Show resolved Hide resolved
@sergeyklay sergeyklay self-requested a review September 6, 2023 09:56
@sergeyklay sergeyklay self-assigned this Sep 6, 2023
@sergeyklay sergeyklay added the enhancement New feature or request label Sep 6, 2023
CHANGELOG.rst Outdated Show resolved Hide resolved
@mschoettle
Copy link

#415 suggested to disable interpolation by default. Would this not be safer to avoid impacting existing users?

@David-Wobrock
Copy link
Contributor Author

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? :)

@mschoettle
Copy link

Thanks for the explanation! I agree that making it explicit in the changelog would be a good thing.

@David-Wobrock
Copy link
Contributor Author

I pushed more documentation in the CHANGELOG @mschoettle
Feel free to rephrase :)

@mschoettle
Copy link

Looks good to me!

tests/test_env.txt Outdated Show resolved Hide resolved
tests/test_env.py Outdated Show resolved Hide resolved
@sergeyklay
Copy link
Collaborator

sergeyklay commented Sep 22, 2023

@David-Wobrock I've merged #500 to fix #499. And now this branch has conflicts that must be resolved. Could you please rebase

@David-Wobrock David-Wobrock force-pushed the feat/add-interpolate-option-with-more-tests branch from 0825d60 to eb50590 Compare September 25, 2023 09:21
@David-Wobrock
Copy link
Contributor Author

Thanks for the review @sergeyklay
I rebased and pushed an additional commit on top of the branch :)

@coveralls
Copy link

coveralls commented Sep 25, 2023

Coverage Status

coverage: 92.281% (+0.01%) from 92.268% when pulling eb50590 on David-Wobrock:feat/add-interpolate-option-with-more-tests into e3e7fc9 on joke2k:develop.

@potasiak
Copy link

potasiak commented Nov 11, 2023

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:

  1. 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.
  2. In April 2023, the mechanism has been removed in favor of a new variable expansion mechanism (v0.11.0) that allowed for using variable interpolation in middle of values, e.g. XXX$VAR_A:YYY$VAR_B.
  3. In July 2023, an interpolate parameter has been added to the Env.__init__() (v0.11.0) because the variable expansion caused issues, a it was not backward compatible.
  4. Then, both the 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.

  1. 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.
  2. 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.
  3. 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.

What I believe should be done (already working on it, I can make a PR once done):

  1. (Maybe) Introduce an 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.
  2. Re-introduce "variable expansion" which is actual interpolation, and use it only when 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

@David-Wobrock
Copy link
Contributor Author

David-Wobrock commented Nov 24, 2023

Hey @potasiak

Thanks for your thorough message.

  1. 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:

  1. The documentation of 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.
  2. We need a way to disable this proxying of values. I want my environment variable that starts with a $ 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.
  1. 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.

  1. 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.

  2. 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 🙂

@David-Wobrock
Copy link
Contributor Author

Hello @potasiak @mschoettle @sergeyklay

Any chance we can move forward on this? :)

@potasiak
Copy link

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.

@David-Wobrock
Copy link
Contributor Author

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 :)

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