CLI dispatch mechanism #108

mailund · 2016-09-14T10:48:14Z

Design for a CLI interface based on installing subcommands and callbacks.

For each subcommand you write a class with two hooks: one for setting sub-command arguments and the other to be called if the sub-command is invoked.

I haven't tested the _split_target_list function in StatusCommand, 'cause I don't know how to muck up a test of target status checking right now.

dansondergaard · 2016-09-21T13:04:46Z

Makefile

@@ -23,7 +23,7 @@ lint:
 	flake8

 coverage:
-	coverage report
+	coverage report --omit gwf/cli/__main__.py


This should be specified in the configuration file .coveragerc instead.

dansondergaard · 2016-09-21T13:10:14Z

gwf/backends/slurm.py

@@ -1,117 +1,212 @@
-from __future__ import absolute_import
-from __future__ import print_function
+from __future__ import absolute_import, print_function


We only support Python 3.4+, so we don't need imports from __future__.

Okay, but that is actually your commit and not mine ;)

dansondergaard · 2016-09-21T13:10:45Z

gwf/cli/__init__.py

@@ -0,0 +1 @@
+from __future__ import absolute_import, print_function


Unnecessary import. See comment above.

dansondergaard · 2016-09-21T13:12:13Z

gwf/cli/__main__.py

@@ -0,0 +1,26 @@
+from __future__ import absolute_import, print_function


I think this file should be called entrypoint.py to make it clear that this is the entry point of the program.

dansondergaard · 2016-09-21T13:27:08Z

gwf/cli/arg_parsing.py

+import argparse
+
+
+class SubCommand(object):


I think just Command is better than SubCommand. Otherwise RunCommand should be called RunSubCommand, but that's just ugly.

Python 3 has new-style classes by default, so there's no reason to inherit from object explicitly.

Fixed the name.

dansondergaard

Great!

I don't know why there's a lot of changes to the SLURM backend included in this PR. Is it possible to remove again, maybe my merging master into the branch again?

Some general comments:

Order imports according to PEP8 (this is done automatically if you use Atom with the atom-beautify plugin).
Remove spacing around =. The above mentioned plugin takes care of that too.
Don't import from __future__.

dansondergaard · 2016-09-21T13:28:29Z

gwf/cli/arg_parsing.py

+from __future__ import absolute_import, print_function
+from ..utils import cache, import_object
+from ..core import PreparedWorkflow
+import argparse


I'd prefer if the file was just called parsing.py to be consistent with PEP8 recommendations about not using underscores in module names. It's also prettier.

Order imports alphabetically with stdlib imports first, then third-party imports, then package imports.

I changed the name of the file and order of imporots.

dansondergaard · 2016-09-21T13:29:09Z

gwf/cli/arg_parsing.py

+    @property
+    @cache
+    def workflow(cls):
+        if cls._workflow is None: # pragma: no cover


As you write, no reason to check this. We'll make sure to always set the workflow.

Removed the check

dansondergaard · 2016-09-21T13:30:09Z

gwf/cli/arg_parsing.py

+    def handle(self, arguments):
+        "Callback when this sub-command is invoked."
+
+class ArgumentDispatching(object):


Don't inherit from object.

I think this should just be called Dispatcher.

Why do you have a problem with inheriting from object? in 2.7 it is the way to use the new class system and it doesn't look like it is doing any damage in 3.5.

True, it's just unnecessary :-)

Ok. Will remove it then

dansondergaard · 2016-09-21T13:30:54Z

gwf/cli/arg_parsing.py

+
+    def __init__(self):
+        self.parser = argparse.ArgumentParser(
+            description="Grid WorkFlow",


Maybe use the same description as in the docs: "A flexible, pragmatic workflow tool."

dansondergaard · 2016-09-21T13:31:44Z

gwf/cli/arg_parsing.py

+
+        # Set global options here. Options for sub-commands will be set by the
+        # sub-commands we dispatch to.
+        self.parser.add_argument("-w", "--workflow",


For backward compatibility I think the flag should be -f and --file. No reason to confuse users unnecessarily.

dansondergaard · 2016-09-21T13:32:39Z

gwf/cli/arg_parsing.py

+        self.subparsers = self.parser.add_subparsers(title = "commands", description = "Subcommands to execute")
+
+    def install_subcommand(self, name, description, handler):
+        """Install a sub-command with "name" and with functionality implemented


Weird wrapping?

Sorry, my mistake. That was my editor wrapping it :-)

dansondergaard · 2016-09-21T13:32:46Z

gwf/cli/arg_parsing.py

+                                 default = "workflow.py")
+
+        # Prepare for sub-commands
+        self.subparsers = self.parser.add_subparsers(title = "commands", description = "Subcommands to execute")


dansondergaard · 2016-09-21T13:34:42Z

gwf/cli/status.py

+from math import ceil
+
+
+__doc__ = """


Doc string should just go to the top of the module (then it will be automatically assigned to __doc__).

dansondergaard · 2016-09-21T13:37:38Z

gwf/exceptions.py

@@ -13,6 +13,16 @@ def __init__(self, target):

        super(TargetExistsError, self).__init__(message)

+class TargetDoesNotExistsError(GWFError):


Grammar: Exists -> Exist.

dansondergaard · 2016-09-21T13:38:35Z

setup.py

    test_suite='tests',
+    install_requires=[
+        "colorama",


Specify a minimum version with >=major.minor.

Bah. I don't know which minimal version to use.

The most recent one right now: 0.3.7 ;)

# Conflicts: # setup.py

@dansondergaard

The unit tests are failing now but it is in the code that prepares the workflow, so I think the problem is with the code I merged in from the master branch which I haven't checked how is changed. @dansondergaard, can you see why there is a key error in a dictionary lookup in the workflow preparation now?

dansondergaard and others added 6 commits August 31, 2016 16:27

Added cli skeleton with begins.

49f32b0

Merge commit '782c13f75f8fd6b2b6770cf8778b233c6cd88132' into new-cli

df545fe

Merge branch 'master' into new-cli

3f82e6a

Rudimentary subcommand dispatch mechanism.

87be76f

Removed default subcommand -- it is not going to work anyway.

16d261d

Forgot to save this file before committing...

a755a80

dansondergaard added the refactor label Sep 14, 2016

dansondergaard added this to the 1.0.0 milestone Sep 14, 2016

dansondergaard assigned dansondergaard and mailund Sep 14, 2016

mailund added 7 commits September 14, 2016 15:10

status output for verbose status.

e23c893

Progress bar status

5ea6e13

Minor aesthetics fix

3640d86

Merge branch 'master' into new-cli

7ec62c7

Testing arg_parsing

688b661

Test of status sub-command.

d75dda5

I haven't tested the _split_target_list function in StatusCommand, 'cause I don't know how to muck up a test of target status checking right now.

A little lint'ing.

0af8ad5

dansondergaard reviewed Sep 21, 2016

View reviewed changes

dansondergaard requested changes Sep 21, 2016

View reviewed changes

mailund and others added 9 commits September 21, 2016 16:31

Merge branch 'master' into new-cli

33bea49

# Conflicts: # setup.py

Fixed (yet another) bug in computation of dependencies.

e660101

Small fixes to parsing.

70010f8

Formatting of tests.

15bb733

Colorama install apparently creates an .eggs dir. Ignore it.

eba7426

Fix linting errors.

2fcd8c7

Add minimum version of colorama.

136953a

Remove explicit inherit from object.

c7cce2c

dansondergaard merged commit c7cce2c into master Sep 23, 2016

dansondergaard deleted the new-cli branch September 23, 2016 11:49

		@@ -0,0 +1 @@
		from __future__ import absolute_import, print_function

		@@ -0,0 +1,26 @@
		from __future__ import absolute_import, print_function

		@@ -13,6 +13,16 @@ def __init__(self, target):

		super(TargetExistsError, self).__init__(message)

		class TargetDoesNotExistsError(GWFError):

CLI dispatch mechanism #108

CLI dispatch mechanism #108

Conversation

mailund commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dansondergaard left a comment

Choose a reason for hiding this comment

dansondergaard Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dansondergaard Sep 21, 2016 •

edited