On Fri, Aug 28, 2015 at 11:17:18AM +0200, Jiri Prochazka wrote:
> This patch introduces several changes to the _query_pool_dir method in lnst.Controller.Wizard:
> - Method was renamed to _check_and_query_pool_dir to better express nature of the method
> - Method now uses 1 optional argument - pool_dir - if set to something else than None,
>   it checks if it's valid path to existing or creatable dir, if it is, uses it,
>   else queries user for a new path (which is also checked)
> - Method noninteractive has added default value (None) for argument pool_dir
>
> This method was reworked to prevent code duplication which would occur when mode for virtual
> machines would be introduced
>
> Signed-off-by: Jiri Prochazka <jprochaz@redhat.com>
> ---
>  lnst/Controller/Wizard.py | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
> index 51856e8..b35b216 100644
> --- a/lnst/Controller/Wizard.py
> +++ b/lnst/Controller/Wizard.py
> @@ -32,22 +32,7 @@ class Wizard:
>          """ Starts Wizard in an interactive mode
>          @param pool_dir Path to pool directory (optional)
>          """
> -        if pool_dir is None:
> -            pool_dir = self._query_pool_dir()
> -        else:
> -            rv = self._check_path(pool_dir)
> -            if rv == PATH_IS_DIR_ACCESSIBLE:
> -                print("Pool directory set to '%s'" % pool_dir)
> -            elif rv == PATH_DOES_NOT_EXIST:
> -                sys.stderr.write("Path '%s' does not exist\n" % pool_dir)
> -                pool_dir = self._query_pool_dir()
> -            elif rv == PATH_NOT_DIR:
> -                sys.stderr.write("Path '%s' exists but is not a directory\n"
> -                                 % pool_dir)
> -                pool_dir = self._query_pool_dir()
> -            elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
> -                sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
> -                pool_dir = self._query_pool_dir()
> +        pool_dir = self._check_and_query_pool_dir(pool_dir)
>
>          while True:
>              hostname = self._query_hostname()
> @@ -75,7 +60,7 @@ class Wizard:
>              else:
>                  break
>
> -    def noninteractive(self, hostlist, pool_dir):
> +    def noninteractive(self, hostlist, pool_dir=None):

1. Why "None" and not "DefaultPoolDir" ?

2. Doesn't it make more sense to split this into it's own patch? It
doesn't look like it's logically connected to anything else changed in
this patch.

>          """ Starts Wizard in noninteractive mode
>          @param hostlist List of hosts (mandatory)
>          @param pool_dir Path to pool_directory (optional)
> @@ -330,13 +315,15 @@ class Wizard:
>                  sys.stderr.write("Hostname '%s' is not translatable into a "
>                                   "valid IP address\n" % hostname)
>
> -    def _query_pool_dir(self):
> +    def _check_and_query_pool_dir(self, pool_dir):
>          """ Queries user for pool directory
> +        @param pool_dir Optional pool_dir which will be checked and used if OK
>          @return Valid (is writable by user) path to directory
>          """
>          while True:
> -            pool_dir = raw_input("Enter path to a pool directory "
> -                                 "(default: '%s'): " % DefaultPoolDir)
> +            if pool_dir is None:
> +                pool_dir = raw_input("Enter path to a pool directory "
> +                                     "(default: '%s'): " % DefaultPoolDir)
>              if pool_dir == "":
>                  pool_dir = DefaultPoolDir
>
> @@ -359,6 +346,7 @@ class Wizard:
>              elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
>                  sys.stderr.write("Directory '%s' is not writable\n"
>                                   % pool_dir)
> +            pool_dir = None
>
>      def _query_port(self):
>          """ Queries user for port
> --
> 2.4.3
>

> _______________________________________________
> LNST-developers mailing list
> LNST-developers@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers