DM-27049: Fix BpsConfig after changes caused warnings #5
Conversation
After recent code changes in other ticket, bpsub started printing bunches of warning messages like the following: WARNING::10/02/2020 18:31:00::Missing search section... This was due to sub-configs being created with same top-level search_order. Modified BpsConfig to take search_order as an optional argument and fixed creation of object in bps_core.py to pass the BPS search_order into the constructor. Also fixed the following while in the bps_config.py code: * Fixed bug when printing an error message that assumed an internal value existed (current). * Fixed function signatures for __getitem__ and __contains__. * Added info to search docstring about valid keys for the search options.
mxk62
left a comment
mxk62
left a comment
There was a problem hiding this comment.
I'm fine with merging these changes to master after addressing few quite minor issues I pointed out in the comments.
python/lsst/ctrl/bps/bps_config.py
Outdated
| if not found and opt.get("required", False): | ||
| print("\n\nError: search for %s failed" % (key)) | ||
| print("\tcurrent = ", Config.__getitem__(self, "current")) | ||
| print("\tcurrent = ", Config.get(self, "current")) |
There was a problem hiding this comment.
If i'm not mistaken,self.get("current") should do here as BpsConfig inherits get() method from its parent class.
python/lsst/ctrl/bps/bps_config.py
Outdated
| replaceVars: `bool`, default = True | ||
| If search result is string, whether to replace variables inside it | ||
| required: `bool`, default = False | ||
| If replacing variables, whether to raise exception if variable is undefined |
There was a problem hiding this comment.
As they say, no good deed goes unpunished...
In my opinion, this entire docstring would benefit from being rewritten and expanded, preferably with some examples. With the opt option, the functionality of search() method goes way beyond mere checking whether a setting was defined in a configuration and what's its value. Yet figuring out the details is close to impossible without looking directly at the function definition itself.
Having said that, I'm fine with doing this work as part of some other, clean-up ticket. However, in this pull request, please address the following issues:
- The type of the
defaultvariable is not specified (line146). - LSST Dev Guide suggests slightly different formatting when describing a dictionary type (lines 144-151). For details follow this link.
- The term hierarchy rules is rather "cryptic", search order may be better option.
Finally, while it is mostly a matter of personal taste, I'm not a huge fan of using abbreviations as "drop-in" replacements for actual words in sentences, proper or not. Words like val or opt works just fine as a variable name, but reading phrases like current vals, given opt in the description feels rather, I don't know, awkward.
There was a problem hiding this comment.
Leaving code examples in the docstring to some other ticket. Otherwise making these changes.
After recent code changes in other ticket, bpsub started printing
bunches of warning messages like the following:
WARNING::10/02/2020 18:31:00::Missing search section...
This was due to sub-configs being created with same top-level
search_order. Modified BpsConfig to take search_order as an
optional argument and fixed creation of object in bps_core.py
to pass the BPS search_order into the constructor.
Also fixed the following while in the bps_config.py code:
internal value existed (current).
search options.