-
Notifications
You must be signed in to change notification settings - Fork 345
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 #2361: allow multiline properties by correctly encoding them #2369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot easily understand the meaning of the toPropertyEntry
function. I'd suggest to include a set of unit test in run_test.go
just for this private function with clear input/output expectation. It will improve the readability and future updates.
pkg/cmd/run.go
Outdated
item = strings.ReplaceAll(item, `=`, `\=`) | ||
item = strings.ReplaceAll(item, `:`, `\:`) | ||
return item | ||
func toPropertyEntry(props *properties.Properties, key string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function is repeated in another unit, I'd avoid to include repeated code.
pkg/cmd/run.go
Outdated
item = strings.ReplaceAll(item, `:`, `\:`) | ||
return item | ||
func toPropertyEntry(props *properties.Properties, key string) (string, error) { | ||
value, _ := props.Get(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can instead provide that value
variable as a input variable of the fuction instead of props
. You only use props
to extract this value AFAICS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I've seen you already did it this way in the other function in common.go
. I'd suggest just using this one then.
Right about refactoring the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, thanks!
Fix #2361
@squakez mind having a look?
Release Note