Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Keep argument value as list #1257

Merged
merged 2 commits into from
Aug 13, 2016
Merged

Conversation

objmagic
Copy link
Contributor

@objmagic objmagic commented Aug 13, 2016

A bug introduced by #1119 takes only the first element of the argument value list and destructively updates the corresponding key argument in the dictionary. See: https://github.com/twitter/heron/pull/1119/files#r74668021

This problem is blocking PyHeron integration test so this is not good. This PR fixed it.

cc: @prabhuinbarajan @taishi8117

@objmagic objmagic added the bug label Aug 13, 2016
@objmagic objmagic self-assigned this Aug 13, 2016
@kramasamy
Copy link
Contributor

👍

@objmagic objmagic merged commit 495a71a into apache:master Aug 13, 2016
@prabhuinbarajan
Copy link
Contributor

sounds good to me... one call out on heronrc - when the same argument is provided at multiple levels, this was supposed to clobber the argument list and enforce the precedence. Now we should find another way to support that or have a convention in place where same argument is not supplied at multiple levels .

@objmagic
Copy link
Contributor Author

when the same argument is provided at multiple levels, this was supposed to clobber the argument list and enforce the precedence

Do you mean for .heronrc file like:

........ --config-property a=b
...
...
........ --config-property c=d --config-property a=b

we should dedup list --config-property a=b --config-property c=d --config-property a=b to --config-property a=b --config-property c=d? Is this what you meant by "clobber"?

Also, can you elaborate what you mean by "precedence"? and why we should preserve it?

nicknezis pushed a commit that referenced this pull request Sep 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants