Refactor how we process --set options#5067
Conversation
| sys.exit(1) | ||
|
|
||
| try: | ||
| # TODO: `--set` should take precedence over config.yaml, but we do this first for now as it may modify confdir. |
There was a problem hiding this comment.
I've created an issue to track this and not forget #5069
| try: | ||
| current = getattr(self.master.options, foc.opt.name) | ||
| d = self.master.options.parse_setval(foc.opt, v, current) | ||
| d = self.master.options.parse_setval(foc.opt, [v]) |
There was a problem hiding this comment.
this method could be used here?
There was a problem hiding this comment.
Not sure if I'm following. Could you explain what you are referring to? This here only handles non-sequence types.
There was a problem hiding this comment.
I was thinking that if there is a set method which accepts a name and a value we don't need to use update.
In general my first impression is that we could make things consistent if we either always expect the caller to do parse_setval or allow two ways of setting options: parsed pair or a raw string (in which case parse_setval becomes a private function?)
|
I've made some changes based on your feedback - thanks again! We now end up funneling everything through |
|
I ran locally the test from #5010 (comment) and it passed successfully. |
|
Thank you again for the initial PR and the very helpful reviews! 🍰 |
refs #5056