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

fix "from env" newline handling #1021

Merged
merged 1 commit into from
Jan 26, 2025
Merged

Conversation

xav-ie
Copy link
Contributor

@xav-ie xav-ie commented Jan 25, 2025

Many (at least the ones I have encountered) env loaders replace "\n" with a
literal newline character when loading an env variable. I need this change in
my personal config and I think others would also prefer this default.

@fdncred
Copy link
Collaborator

fdncred commented Jan 25, 2025

I wonder how this will work on Windows since Windows uses \r\n.

@xav-ie
Copy link
Contributor Author

xav-ie commented Jan 25, 2025

hmm. that is good question.

Here is what I find from other env loader:
https://github.com/dotenvx/dotenvx/blob/79b04e3e46d62362e2ad2415bd8aca91f59db87f/src/lib/helpers/parse.js#L123-L127
^ It looks like we should also replace carriage return and tabs, too!
This is a good catch. I will update pr now

Many .env loaders replace `\n`, `\r`, `\t`. Follow the standard of properly
replacing with their literal equivalents when loading the variable for
use.
@xav-ie xav-ie force-pushed the fixFromEnvNewLine branch from d22181f to a7968ad Compare January 25, 2025 23:58
@xav-ie
Copy link
Contributor Author

xav-ie commented Jan 25, 2025

I updated the PR. I am happy to amend the right hand of the replacement to use (char newline), (char carriage_return), and (char tab). Please just let me know! I appreciate the feedback.

@fdncred
Copy link
Collaborator

fdncred commented Jan 26, 2025

I'm not sure how this'll work but let's try it.

@fdncred fdncred merged commit e245718 into nushell:main Jan 26, 2025
1 check failed
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.

2 participants