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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

BrentWilkins
Copy link

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
Copy link

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.

@github-actions github-actions bot changed the base branch from main to develop September 18, 2023 21:08
@BrentWilkins BrentWilkins force-pushed the Allow_split_variable_definitions_and_exports branch from b3292fd to 8c526cc Compare September 19, 2023 18:41
@sergeyklay
Copy link
Collaborator

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants