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 join #495

Merged
merged 2 commits into from
Aug 24, 2017
Merged

Fix join #495

merged 2 commits into from
Aug 24, 2017

Conversation

ahelal
Copy link
Contributor

@ahelal ahelal commented Aug 23, 2017

Fixes #494

core/flags.go Outdated
@@ -33,7 +33,7 @@ func (f *MultiFlag) Set(value string) error {
return fmt.Errorf(
"flag value '%v' was not in the format 'key=val'", value)
}
key, val := strings.Join(pair[0:1], ""), strings.Join(pair[1:2], "")
key, val := strings.Join(pair[0:1], ""), strings.Join(pair[1:], "=")
Copy link
Contributor

Choose a reason for hiding this comment

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

With fresh eyes the original line here looks questionable to me and I'm not sure what it's supposed to be getting us. Couldn't we do?:

func (f *MultiFlag) Set(value string) error {
 	if f.Len() == 0 {
 		f.Values = make(map[string]string, 1)
 	}
	pair := strings.SplitN(value, "=", 2)
	if len(pair) < 2 {
		return fmt.Errorf("flag value '%v' was not in the format 'key=val'", value)
	}
	f.Values[pair[0]] = pair[1]
	return nil
}

Do we have an example where the original line does something we need? It'd be nice to have a unit test for this.

@ahelal
Copy link
Contributor Author

ahelal commented Aug 24, 2017

Hi @tgross,

I update the PR. I attempted to add a test, but was not sure I am in the right direction.
I wanted to check for -putenv ENV_VALUE=PART1 and -putenv ENV_VALUEPART1 to cover all cases. I kind of have the urge to test Set directly instead of using os.Args and then GetArgs()

But feeling lost somehow. So your input would be helpful.

@tgross
Copy link
Contributor

tgross commented Aug 24, 2017

I think this LGTM. Let's get it in!

@tgross tgross merged commit c326eda into TritonDataCenter:master Aug 24, 2017
@ahelal ahelal deleted the fix/indices branch August 24, 2017 12:54
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