DM-24584: create an ingestRaws butler command #279
Conversation
|
Quick comment: For the comment with message "add support for ingest-raw command" -- daf_butler doesn't really care about ingest-raw in obs_base and the commit doesn't seem to be immediately obvious as being about ingest-raw. Can you change the commit message to be more relevant to what is really going on in daf_butler? |
| if "DAF_BUTLER_MOCK" in os.environ: | ||
| printFunctionInfo(func, *args, **kwargs) | ||
| return | ||
| try: |
There was a problem hiding this comment.
I tried to implement this with a unittest.mock, but couldn't find a way to get the called mock object back out to the calling script for verification. I couldn't pass it in because butler wants to import the command, and even when replacing the object in sys.modules, import still provides a copy of the class, not the class itself :-(
the handler should be used to call command implementation functions.
This conforms with the standard Click argument name.
|
Wasn't I supposed to be reviewing this PR? |
There was a problem hiding this comment.
Looks fine overall. If the cli wrappers are only going to be minimalist and never have business logic in them there is no real reason to split them into separate files in the cli tree unless the import overhead is really high. Did only one of the commands get relocated to scripts?
| # implementation function it should print details about the called command to | ||
| # stdout. These details are then used to verify the command function was loaded | ||
| # and received expected inputs. | ||
| DAF_BUTLER_MOCK = {"DAF_BUTLER_MOCK": ""} |
There was a problem hiding this comment.
It reads like the variable here is the contents of the environment variable but environment variables can't be a dict so is the key in the dict the environment variable name and the value in the dict what is set by the user? Having the global use the same name as the key is confusing here.
| Raised if the separator is not found in an entry, or if duplicate keys | ||
| are encountered. | ||
| """ | ||
| if "," == separator or " " == separator: |
There was a problem hiding this comment.
Sometimes for future expansion possibilities it's better to write this as
if separator in (",", " "):| The names and values of the kwargs that should have been passsed to the | ||
| funciton. | ||
| """ | ||
| calledWith = eval(output) |
There was a problem hiding this comment.
if we are going to be using eval on the output that we generated above, I think it would be safer all around if we made the "print" do a yaml dump and here we did a yaml safe_load.
|
|
||
| Parameters | ||
| ---------- | ||
| testSuite : `unittest.Testsuite` |
There was a problem hiding this comment.
Are you sure this isn't a TestCase rather than a TestSuite?
|
@timj, I did only move one command function into script. I didn't want to do all of them without approval on the concept so I did this one to demonstrate. Since it seems like you agree with the concept I'll make a ticket to do the other butler commands. |
No description provided.