Skip to content

Add multi-line values support#118

Closed
x1unix wants to merge 2 commits into
joho:mainfrom
x1unix:feat/value-separator
Closed

Add multi-line values support#118
x1unix wants to merge 2 commits into
joho:mainfrom
x1unix:feat/value-separator

Conversation

@x1unix

@x1unix x1unix commented Sep 10, 2020

Copy link
Copy Markdown
Contributor

Fixes #117 issue with multi-line values parsing.

Had to refactor existing parser to implement this.
Theoretically, a new parser should be faster, since less iterations over source slice are required to do parsing.

The new parser passes all tests in godotenv_test.go, so it doesn't introduce any breaking changes.

Upd: If anyone needs this fix, you can a branch with fix or just use the latest tag from my fork.

Replace approach

replace (
	github.com/joho/godotenv => github.com/x1unix/godotenv v1.3.1-0.20200910042738-acd8c1e858a6
)

Different package approach

go get -u github.com/x1unix/godotenv@v1.4.0

@joho

joho commented Nov 8, 2020

Copy link
Copy Markdown
Owner

I would like to support multiline. Do you know if this behaviour is consistent with the ruby & node dotenv libraries?

@x1unix

x1unix commented Nov 8, 2020

Copy link
Copy Markdown
Contributor Author

@joho I don't use ruby or node so I don't know, but this is a correct behavior for environment variables in shell scripts.

@x1unix

x1unix commented Nov 10, 2020

Copy link
Copy Markdown
Contributor Author

@joho tested node package dotenv - it has the same issue with multiline values:

Screenshot_20201110_174640

@x1unix

x1unix commented Nov 10, 2020

Copy link
Copy Markdown
Contributor Author

@joho here is a reference behavior (shell script):

image

@x1unix

x1unix commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

@joho any news regarding this PR?

@joho

joho commented Dec 23, 2020

Copy link
Copy Markdown
Owner

Sorry for the delay, I'm planning on doing a maintenance pass over all my outstanding GitHub over the Xmas break

@coolaj86

coolaj86 commented Mar 8, 2021

Copy link
Copy Markdown
Contributor

@joho I know you're busy. Is there any sort of help that someone else could give to get this moving forward?

@x1unix

x1unix commented Mar 8, 2021

Copy link
Copy Markdown
Contributor Author

@coolaj86 currently I'm just using my fork until this PR is "on review":

You can temporary redirect dependency to a fork in go.mod:

replace (
  github.com/joho/godotenv => github.com/x1unix/godotenv v1.3.1-0.20200910042738-acd8c1e858a6
)

@x1unix

x1unix commented Aug 2, 2021

Copy link
Copy Markdown
Contributor Author

@joho any news?

@milon-bs23

Copy link
Copy Markdown

any update regarding this PR?

… declaration

Signed-off-by: x1unix <denis0051@gmail.com>
Signed-off-by: x1unix <denis0051@gmail.com>
@x1unix x1unix force-pushed the feat/value-separator branch from acd8c1e to 993ff7a Compare September 23, 2021 21:22
@x1unix

x1unix commented Sep 23, 2021

Copy link
Copy Markdown
Contributor Author

@milon-bs23 you can switch to my branch until this PR is not merged:

go.mod

replace (
	github.com/joho/godotenv => github.com/x1unix/godotenv v1.3.1-0.20200910042738-acd8c1e858a6
)

Also, you can try to use the latest version from by repo (v1.4.0) but it has a different package URL (x1unix/godotenv).

@joho

joho commented Sep 24, 2021

Copy link
Copy Markdown
Owner

I'm doing some manual testing on my end to ensure this change is in line with https://github.com/bkeepers/dotenv/blob/master/spec/dotenv/parser_spec.rb#L198-L234

If so, I'll merge and cut a release.

@joho joho mentioned this pull request Sep 24, 2021
@joho

joho commented Sep 24, 2021

Copy link
Copy Markdown
Owner

@x1unix in testing your PR I've found a problem with windows newlines.

To give myself extra piece of mind that your multiline behavior matches the canonical ruby implementation I copied a few of their testcases over into fixtures/quoted.env. They work find on linux & mac, but fail the build on windows.

You can see what I've done on #156

I have some unrelated build cleanup work to do which I'll pull into another PR, but if you can get the windows linebreak issue resolved I can merge whenever.

Comment thread parser.go
// this differs from unicode.IsSpace, which also applies line break as space
func isSpace(r rune) bool {
switch r {
case '\t', '\v', '\f', '\r', ' ', 0x85, 0xA0:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment and the code disagree. \r (carriage return) is generally considered to be a newline. Technically it is "return to the 0th position of the current row", but I that's for old printers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@coolaj86 \r character is included here for a Windows support. Windows has a different new line separator - \r\n.

@x1unix

x1unix commented Sep 24, 2021

Copy link
Copy Markdown
Contributor Author

@joho thank you for the reply, I'll take a look at Windows build in a few days :)

@decentral1se

decentral1se commented Jan 18, 2022

Copy link
Copy Markdown

Nice patch, works nicely but I've noticed that it breaks preserving inline comments :(

@austinsasko

Copy link
Copy Markdown
Contributor

@joho what remains for this PR?

@acastro2

Copy link
Copy Markdown

Can we get this merged, please?

@joho

joho commented Jan 27, 2023

Copy link
Copy Markdown
Owner

This PR is now indirectly merged via #156

I'm going to leave it on main without a numbered release for a little while (or do a beta release number) to let any regressions fall out first.

@joho joho closed this Jan 27, 2023
@joho

joho commented Feb 5, 2023

Copy link
Copy Markdown
Owner

So this is really on me for not reviewing close enough but there were a couple of problems in this PR.

The first was in the category of what I expected and was procrastinating about - essentially having undefined behaviour I hadn't written a test for that would be missed with the new parser. #204 was it.

In trying to fix it in #205, I noticed the bigger problem.

The new parser replaced the existing parser, but didn't clean out the old private functions, and not all of the tests were updated to point at the new parser. This was particularly problematic as TestParsing has all the odd edge cases and wasn't updated. I've updated all tests to call the newer code paths (and deleted the old stale paths) and there's a few more failing cases.

I'll try and get the tests passing again at some point this week but I've got a pretty full schedule, so if anyone involved here wanted to finish #205 off sooner I'd appreciate it.

@joho

joho commented Feb 5, 2023

Copy link
Copy Markdown
Owner

Managed to fix it last night, no need for follow up. thanks ✌️

@x1unix x1unix deleted the feat/value-separator branch February 7, 2023 00:30
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.

Library can't parse multiline values properly

7 participants