Fixes #64 Multiline variables #65
Conversation
|
Hold on reviewing; I identified an edge case that needs to be addressed first |
|
Any update on this? Also experiencing this issue. What was the edge case you found? |
|
@kiambogo this fix works perfect unless you have Unfortunately the current implementation for identifying keys and values is pretty naive; I tried «piggy backing» on top of the current implementation and just tried to add support for multi-line; the problem is that this implementation relies on finding the next «key-value pair» in order to close the multiline value; thus, if you have a |
|
Thanks for the info @esttorhe ! Bummer, as I'm trying to add a PEM encoded RSA private key which indeed includes |
|
@kiambogo same use case as me; that's why I added the support and noticed the issue. I think the naive implementation could still be used for a first version just requiring the use of either |
|
@joho Any chance to get a review? |
There was a problem hiding this comment.
Sorry it took me a while to review. Unfortunately I don't want to merge as-is.
I think in general the tests are fine and the string juggling within parseLine (though I think you'll need to split parseLine into two functions, one for handling single lines and another for handling multilines.
The logic in Parse is where it needs the most work - I can't follow what it does very well as a reader and so I'm not confident it's free of edge cases etc. An design approach that might work is to have the regular parseLine return a specific error stating that the line is the beginning of a multiline, and have an error check. If it matches, pass that line and any subsequent lines into a parseMultiline until that hits the end of the value.
Other approaches can definitely work but I just want to see less jumbled special case logic up in Parse if possible.
| @@ -209,7 +246,7 @@ func readFile(filename string) (envMap map[string]string, err error) { | |||
| return Parse(file) | |||
| } | |||
|
|
|||
| func parseLine(line string, envMap map[string]string) (key string, value string, err error) { | |||
| func parseLine(line string, envMap map[string]string, isMultiline bool) (key string, value string, multilineValue bool, err error) { | |||
There was a problem hiding this comment.
I'd appreciate if you could find a way to restructure this to not rely on a boolean flag as I'm in Fowler's camp on the topic https://www.martinfowler.com/bliki/FlagArgument.html
|
|
||
| if err != nil { | ||
| return | ||
| } | ||
| envMap[key] = value | ||
|
|
||
| if multilineValue == true && isMultilineValue == false && len(key) <= 0 { |
There was a problem hiding this comment.
I have to admit I'm really confused by the two multilineValue booleans, and I'm not sure if that's a structural problem or a naming problem of the variables.
There was a problem hiding this comment.
Yeah; this is a fair comment. I'm not particularly happy with the implementation and thus wanted to get some feedback from you since is your codebase.
I'll close this PR and will refactor in a cleaner way probably this weekend.
🙇
Fixes #64
This
PRhandles multi-line values by single quote delimiter'.The parsing of the whole
.envfile is pretty naive (as expressed in theREADME) but does the work. I tried to add as little code as possible and change as little as possible just to support this.All tests are passing and 3 new ones were added (1 success and 2 different failing scenarios)