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 default expansion. #39

Merged
merged 6 commits into from Jan 17, 2022
Merged

Conversation

vantreeseba
Copy link
Contributor

@vantreeseba vantreeseba commented Mar 10, 2020

Allow expansion to occur, but replace non existing with default if
given.

fixes #11

Example: ${MACHINE-localhost}

Should resolve to $MACHINE expansion, and if not set, then expands to
localhost.

Allow expansion to occur, but replace non existing with default if
given.

Example: ${MACHINE-localhost}

Should resolve to $MACHINE expansion, and if not set, then expands to
localhost.
@vantreeseba
Copy link
Contributor Author

vantreeseba commented Mar 10, 2020

There are a few more test conditions I'm adding to ensure no issues occur.

@vantreeseba
Copy link
Contributor Author

This should be good to, let me know if there is anything else you feel like should be tested / etc.

@snyamathi
Copy link

Would using :- make more sense to mimic the shell syntax?

${parameter:-word}
Use Default Values. If parameter is unset or null, the expansion of word shall be substituted; otherwise, the value of parameter shall be substituted.

https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02

@vantreeseba
Copy link
Contributor Author

Would using :- make more sense to mimic the shell syntax?

${parameter:-word}
Use Default Values. If parameter is unset or null, the expansion of word shall be substituted; otherwise, the value of parameter shall be substituted.

https://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02

Yeah, that makes more sense, I guess I missed that somehow, and in bash (at least 4.4.20) it works without the ":", so ${non_existing-default} will return default.

I'll look into changing it.

@paulrostorp
Copy link

any updates on this ?

@vantreeseba
Copy link
Contributor Author

Hey sorry, I honestly completely forgot about this, I will try to look into it this week.

@vantreeseba
Copy link
Contributor Author

With those two latest commits, this should be good to go.
Tests all are passing, let me know if there is anything else you would like.

@elie-g
Copy link

elie-g commented May 24, 2021

Any updates?

@FezVrasta
Copy link
Contributor

Anything holding back on this PR?

@motdotla
Copy link
Owner

Great feature. Thank you. Merging and this will be a major upgrade release shortly.

@motdotla motdotla merged commit 0ec06b8 into motdotla:master Jan 17, 2022
@motdotla
Copy link
Owner

Thank you 💛. This has been released: https://github.com/motdotla/dotenv-expand/blob/master/CHANGELOG.md#600-2022-01-17

@vantreeseba vantreeseba deleted the add_default_expansion branch January 19, 2022 00:41
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Mar 3, 2022
We currently use `dotenv-expand@5` in `@next/env`, which does not support default expansion. It was added in v6 (motdotla/dotenv-expand#39).

Upgrading to the latest version of `dotenv-expand` and fixing an import, the added test passed.

Fixes #34718

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
UNDEFINED_EXPAND_WITH_DEFINED_NESTED=${UNDEFINED_ENV_KEY:-${MACHINE:-default}}
UNDEFINED_EXPAND_WITH_DEFAULT=${UNDEFINED_ENV_KEY:-default}
UNDEFINED_EXPAND_WITH_DEFAULT_NESTED=${UNDEFINED_ENV_KEY:-${UNDEFINED_ENV_KEY_2:-default}}
UNDEFINED_EXPAND_WITH_DEFAULT_NESTED_TWICE=${UNDEFINED_ENV_KEY:-${UNDEFINED_ENV_KEY_2${UNDEFINED_ENV_KEY_3:-default}}}
Copy link
Contributor

@FezVrasta FezVrasta Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vantreeseba may you help me understand how is this supposed to work? Why are UNDEFINED_ENV_KEY2 and ${UNDEFINED_ENV_KEY_3:-default} being concatenated?

Should ${MACHINE${MACHINE}} resolve to machinemachine?

This syntax doesn't seem to work on Bash, I get bad substitution. The correct one should be this I think:

UNDEFINED_EXPAND_WITH_DEFAULT_NESTED_TWICE=${UNDEFINED_ENV_KEY:-${UNDEFINED_ENV_KEY_2}${UNDEFINED_ENV_KEY_3:-default}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That actually looks like my test was missing a :-

So it should look like

UNDEFINED_EXPAND_WITH_DEFAULT_NESTED_TWICE=${UNDEFINED_ENV_KEY:-${UNDEFINED_ENV_KEY_2:-${UNDEFINED_ENV_KEY_3:-default}}}

Idea being, it's a n, n+1,n+2 test to do a "proof by induction".

I can fix it, or if you want to fix it in your branch is fine, whichever everyone thinks will be easier/better.

It passed due to the empty env vars being resolved to empty strings, so it probably should have more test cases like the previous defined/nested undefined ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in my branch, thanks for confirming!

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.

Setting default values if not present in environment
6 participants