Added suport for multiline variables from sh #64
Conversation
smyrman
force-pushed
the
multiline
branch
4 times, most recently
from
06d2240 to
1c29cc9
Compare
|
Thanks again.
I'm a bit in doubt about what approach to push. I kind of dislike these boolean flags, they're a bit too verbose. Now we have two, in the future, if we have more use cases, then we'll have many? This is why I thought about turning it in a slice. Then the use can programmatically do exactly what they want. No flag needed.
I think in these cases we would just push this string as the first element of the slice, to keep the current behavior. I want your opinion if that would solve your use case from #63. |
|
OK, let's do the simplest thing. One can just split the result if they want, as you suggested before
|
Alright. 👍 I can add some convenience functions to make it easy to concat lines or split them (including Windows styled newlines).
I guess that makes sense. That should make most command output work as expected (there is usually just one trailing newline), but leave some leeway for plausible use cases where you would want some of the whitespace kept, e.g. code generation. |
Maybe something for the future, since even on Windows files often use About the trimming, maybe always trim the last line break would be better because:
|
Yes Agreed.
Yeah, but you just need to replace "\r\n" with "\n" and then do the split. This is similar to how e.g. the Go line scanner works, although it uses "peek" to be more memory efficent. I wrote some, but a bit unsure about the best way to name them, as go-taks appears to use |
| "splitLines": func(s string) []string { | ||
| s = strings.Replace(s, "\r\n", "\n", -1) | ||
| return strings.Split(s, "\n") | ||
| }, |
There was a problem hiding this comment.
If you prefer the PascalCase, I can rename them, or if you prefer consistency with sprig, we can rename all of them, but leave backwards compatibility:
taskFuncs := template.FuncMap{
(...)
"fromSlash": func(path string) string {
return filepath.FromSlash(path)
},
"toSlash": func(path string) string {
return filepath.ToSlash(path)
},
"exeExt": func() string {
if runtime.GOOS == "windows" {
return ".exe"
}
return ""
},
// IsSH is deprecated.
"IsSH": func() bool { return true },
}
// Deprecated aliases for renamed functions.
taskFuncs["FromSlash"] = taskFuncs["fromSlash"]
taskFuncs["ToSlash"] = taskFuncs["toSlash"]
taskFuncs["ExeExt"] = taskFuncs["extExt"]We can comment the backwards compatibility only very briefly in the README.
There was a problem hiding this comment.
Agree with renaming them. The reason this is not consistent is because spring was added after some of the other funcs.
There was a problem hiding this comment.
Yeah, I assumed it was historical reasons:-)
| key, val := keyVal[0], keyVal[1] | ||
| m[key] = Var{Static: val} | ||
| } | ||
| return m |
There was a problem hiding this comment.
Did some re-ordering in this file to get a better overview. Basically the ordering rule is to place methods and creator functions right below the type definitions.
Don't know if you agree with this ordering, or if you prefer a different style @andreynering
|
|
||
| // UnmarshalYAML implements yaml.Unmarshaler interface | ||
| // UnmarshalYAML implements yaml.Unmarshaler interface. |
There was a problem hiding this comment.
The trailing period is recommended by Effective Go, or at least the way I interpret it:
Doc comments work best as complete sentences, which allow a wide variety of automated presentations. The first sentence should be a one-sentence summary that starts with the name being declared.
The standard lib uses a period consistently throughout all libraries as far as I have seen. Note that I am only talking about doc comments, not code comments.
variables.go
Outdated
| var sh struct { | ||
| Sh string | ||
| } | ||
| var sh struct{ Sh string } |
smyrman
force-pushed
the
multiline
branch
2 times, most recently
from
e44fa3d to
e422ac3
Compare
variables.go
Outdated
| // Trim a single trailing newline to make the result easier to use in shell | ||
| // commands. | ||
| if strings.HasSuffix(result, "\n") { | ||
| result = result[0 : len(result)-1] |
There was a problem hiding this comment.
result = strings.TrimSuffix(result, "\n")If also not required.
|
Two notes, then you can merge this. Thank you for your work and patience. Feel free to push directly to master if/when you have small fixes and are confident. Specially for documentation and comments. |
Instead of giving an error on multiline results from sh, the results are now stored as is, except that the last newline is stripped away to make the output of most commands easy to use in shell commands. Two helper functions have been added to help deal with multi-line results. In addition, previous PascalCase template function names have been renamed to camelCase for consistency with the sprig lib.
Fixes #63.
Instead of giving an error on multiline results from sh, the results are now concatenated to one line by default, separated by a single space. Trailing and leading white space are stripped like before.In adition, some flags have been added for advanced use cases to turn off line concationaton and/or trimming off leading/trailing white-space.Multiline results are now allowed, while the last newline is stripped to allow the output of most commands to work better as variable values. Some helper functions are added to make it easier to deal with multi-line variables in various ways.