Refactor CLI options #115
Refactor CLI options #115
Conversation
Why these changes are being introduced: * Refactor CLI options so that each CLI command only receives the necessary parameters. How this addresses that need: * Update CLI commands to only add necessary parameters for each command * Refactor Workflow class to only require batch_id for instantiation * Create metadata_mapping_path, s3_bucket, and output_queue properties * Refactor collection_handle and email_recipients as method args * Update workflow fixtures to account for new class properties * Update CLI tests to account for shifting CLI options * Update outdated values in Workflow class unit tests and * Remove unnecessary Workflow class unit test after class attributes are removed Side effects of this change: * None
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
Other than a question about the accuracy of --email-recipients being an override, it looks great!
One more suggestion, but not required, would be to add the CLI --help output to the README. I have a feeling this is somewhat contreversial as a practice, but I find it pretty helpful.
I think painting a picture of the CLI interface in the README users unfamiliar with the application, but also can surface potetially rough edges during development.
For example, I left my comment about --batch-id after running pipenv run dsc --help and observing the output. Yes, I could have caught it in the click CLI code itself... but I missed it there at first.
But totally optional, and could wait.
Pull Request Test Coverage Report for Build 13162662894Details
💛 - Coveralls |
jonavellecuerdo
left a comment
jonavellecuerdo
left a comment
There was a problem hiding this comment.
This is looking really awesome! I agree with @ghukill 's suggestions for updated CLI descriptions.
Also, I can't recall why we created the Demo workflow. If still needed, can you add the @property methods, or if no longer needed, let's remove the following:
dsc/workflows/demo.pydsc/workflows/metadata_mapping/demo.json
| @property | ||
| def output_queue(self) -> str: | ||
| return "awaiting AWS infrastructure" |
There was a problem hiding this comment.
[Non-blocking] Since we are planning to create an output queue for every workflow, this might resemble something like dss-sccs-output, right?
There was a problem hiding this comment.
You are very likely correct but I think we'll need a PR for updating these values once the AWS infra is done so I'd rather not guess until they've been created
* Update parameter docstrings for great accuracy * Add properties to temporary Demo workflow class
Purpose and background context
Refactor CLI commands and
Workflowclass attributes and properties to improve the clarity of the applicationIncludes new or updated dependencies?
YES
Changes expectations for external applications?
NO
Developer
Code Reviewer(s)