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

Handle escaped dollar sign in values #333

Merged
merged 2 commits into from
Oct 7, 2021
Merged

Conversation

mehdy
Copy link
Contributor

@mehdy mehdy commented Oct 6, 2021

Fixes #271

@mehdy mehdy force-pushed the develop branch 4 times, most recently from 1a70e2f to 1e4dadc Compare October 6, 2021 14:09
@coveralls
Copy link

coveralls commented Oct 6, 2021

Coverage Status

Coverage increased (+0.07%) to 90.998% when pulling 1b6c4ba on mehdy:develop into 7dc1c1b on joke2k:develop.

@mehdy
Copy link
Contributor Author

mehdy commented Oct 6, 2021

@sergeyklay ready for review.

@mehdy mehdy changed the title Handle escaped $ in values Handle escaped dollar sign in values Oct 6, 2021
@sergeyklay
Copy link
Collaborator

@mehdy Thank you for the patch.

In fact, I'd like to go this way:

  1. Make current interpolation feature optional and provide an ability to disable it v0.8.0
  2. Implement a brand new interpolation feature using the following form: FOO=${BAR} v0.9.0
  3. Disable by default old feature v0.9.0
  4. Enable by default new feature v1.0.0
  5. Drop old feature v1.0.0

I'm trying to understand how much your patch might hinder or help with this plan.

@sergeyklay sergeyklay self-assigned this Oct 6, 2021
@mehdy
Copy link
Contributor Author

mehdy commented Oct 6, 2021

@sergeyklay You're welcome!

I think I can add a feature flag to the escaping logic so it meets the first step's requirements.
Should I go for it?

@sergeyklay
Copy link
Collaborator

Yeah, it would be better. Also could you disable it by default?

Signed-off-by: Mehdy Khoshnoody <mehdy.khoshnoody@gmail.com>
@mehdy
Copy link
Contributor Author

mehdy commented Oct 7, 2021

@sergeyklay What do you think of this?

@sergeyklay sergeyklay added the hacktoberfest-accepted See https://hacktoberfest.digitalocean.com/ for participants details label Oct 7, 2021
@sergeyklay
Copy link
Collaborator

@mehdy Looks as I expected, thanks! Could you please add a quick note to the documentation? Feel free to edit the tips.rst to show possible use cases and examples.

@mehdy
Copy link
Contributor Author

mehdy commented Oct 7, 2021

@sergeyklay Sure!

@mehdy
Copy link
Contributor Author

mehdy commented Oct 7, 2021

@sergeyklay I think we're good to go!

Signed-off-by: Mehdy Khoshnoody <mehdy.khoshnoody@gmail.com>
@sergeyklay sergeyklay merged commit ef15435 into joke2k:develop Oct 7, 2021
@sergeyklay
Copy link
Collaborator

Thank you for the patch, and for helping make django-environ better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted See https://hacktoberfest.digitalocean.com/ for participants details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants