Scottx611x/configurable branch for vis tool registry #2942
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2942 +/- ##
===========================================
- Coverage 60.67% 59.25% -1.43%
===========================================
Files 433 433
Lines 28103 27107 -996
Branches 1274 1274
===========================================
- Hits 17051 16061 -990
+ Misses 11052 11046 -6
Continue to review full report at Codecov.
|
| raise CommandError("`--branch` requires `--visualizations` to be " | ||
| "specified") | ||
| if options["branch"]: | ||
| self.visualization_registry_branch = options["branch"] |
There was a problem hiding this comment.
I would do less procedural validation and use more of what argparse provides, like argument_group and mutually_exclusive_group.
refinery/tool_manager/tests.py
Outdated
| def test_load_tools_command_error_if_branch_and_workflows_specified(self): | ||
| with self.assertRaises(CommandError): | ||
| call_command("load_tools", workflows=True, | ||
| branch="fake-branch-name") |
There was a problem hiding this comment.
And in a similar vein, if we were using more of the built-in machinery, I don't think we'd need explicit checks on their error handling.
| action='store', | ||
| dest='branch', | ||
| help='Branch from the registry to target for Vis tool installation' | ||
| ) |
There was a problem hiding this comment.
Maybe make the "master" default explicit here, rather than the init? The help text will be more explicit, and you can get rid of one if
…own error handling
…rable_branch_for_vis_tool_registry
There was a problem hiding this comment.
With mutually exclusive options at the top level, subparsers might be useful. I haven't actually used them before, but if it's ok with you I'll check out this branch and and try doing it that way.
|
@mccalluc sounds good to me! |
mccalluc
deleted the
scottx611x/configurable_branch_for_vis_tool_registry
branch
@mccalluc
./manage.py load_tools --visualizations multiqc heatmap-scatterplot --branch new_tool_versions