search

joke2k / django-environ

Django-environ allows you to utilize 12factor inspired environment variables to configure your Django application.
https://django-environ.rtfd.org
MIT License
2.96k stars 315 forks source link

Allow separate declaration and assignment to avoid masking return values #502

Open BrentWilkins opened 9 months ago

BrentWilkins commented 9 months ago

In a project, I was trying to follow a best practice of assigning values to variables on different lines than where I export them. Seems that it's possible to have a bad return code from my_func ignored if it's all on the same line. I don't know if anything is going to try and catch the failure here, but why prevent it? Here is an example of code that was being called bad:

MY_NAME="$(my_func)"
export MY_NAME

I haven't exhaustively tested the change, but it does pass the existing tests. The value in my code is made available where I want it.

github-actions[bot] commented 9 months ago

Your PR was set to target main, PRs should be target develop The base branch of this PR has been automatically changed to develop, please check that there are no merge conflicts.

sergeyklay commented 9 months ago

Hi @BrentWilkins,

First of all, thank you for your contribution to the project. I appreciate the time and effort you've put into this pull request.

After reviewing your changes, I have some concerns:

  1. Specific Use Case: Your PR seems to address a very specific use case related to bash scripting. However, the primary use of this library is to handle .env files, which are generally simple key-value pairs. The need for handling complex bash-like assignments is not clear.
  2. Regex Concerns: The proposed change appears to be somewhat lenient in what it allows, potentially even accepting invalid lines like FOO BAR (without an equal sign), which could lead to unpredictable behavior.
  3. Testing and Documentation: You mentioned that you haven't exhaustively tested the change. Comprehensive tests are crucial for any changes to ensure they don't break existing functionality. Additionally, the PR lacks documentation to justify the need for this feature.
  4. Community Feedback: There hasn't been much discussion or reviews from other contributors, which usually helps in understanding the necessity and impact of a change.

Given these points, I'm leaning towards not merging this PR in its current state. However, I'm open to further discussions and clarifications.