[PATCH] Make the firstboot mixin spokes more mixable.

Vratislav Podzimek vpodzime at redhat.com
Fri Jan 10 09:34:17 UTC 2014


On Thu, 2014-01-09 at 16:17 -0500, David Shea wrote:
> Instead of duplicating the firstboot should_run code in classes that
> need to override it, note the expected inheritance order in the
> firstboot mixin classes and use super calls for most of the should_run
> logic.
Sorry, but I don't quite understand the purpose of these changes. From
my point of view they only make the code more cryptic with the same
result. More below.

> ---
>  pyanaconda/ui/common.py          | 12 ++++++++++--
>  pyanaconda/ui/gui/spokes/user.py | 16 +++++-----------
>  pyanaconda/ui/tui/spokes/user.py | 17 +++++------------
>  3 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/pyanaconda/ui/common.py b/pyanaconda/ui/common.py
> index 63a7afc..7fa3638 100644
> --- a/pyanaconda/ui/common.py
> +++ b/pyanaconda/ui/common.py
> @@ -113,6 +113,9 @@ class UIObject(object):
>  class FirstbootSpokeMixIn(object):
>      """This MixIn class marks Spokes as usable for Firstboot
>         and Anaconda.
> +
> +       Unlike other mixins, this class should be first in the inheritance list
> +       so that it overrides Spoke.should_run.
>      """
>      @classmethod
>      def should_run(cls, environment, data):
> @@ -126,7 +129,8 @@ class FirstbootSpokeMixIn(object):
>             the data argument.
>          """
>  
> -        if environment == ANACONDA_ENVIRON:
> +        # First check the superclass method
> +        if super(FirstbootSpokeMixIn, cls).should_run(environment, data):
That should/will result in running Spoke class' should_run static
method, right? I believe it deserves a huge comment on how that happens
at least for me and the other people who are not 100% familiar with
super behaviour of Python's 'super' function. The Spoke class definitely
doesn't look like a superclass in scope of the FirstbootSpokeMixIn
class.

And while 'if environment == ANACONDA_ENVIRON' is clear and says that
every FirstbootSpokeMixIn instance should also run in the Anaconda, the
'if super(FirstbootSpokeMixIn, cls).should_run(environment, data)' does
the same but in a highly cryptic way, I think. If there were more
environments and more types of classes and inheritance than we have, it
would make sense to have this more sophisticated (from the design's
perspective) code. However, in our simple use case while having all
those classes and spokes under control, it really seems an unnecessary
cryptic way to do things.

>              return True
>          elif environment == FIRSTBOOT_ENVIRON and data is None:
>              # cannot decide, stay in the game and let another call with data
> @@ -143,7 +147,11 @@ class FirstbootSpokeMixIn(object):
>  
> 
>  class FirstbootOnlySpokeMixIn(object):
> -    """This MixIn class marks Spokes as usable for Firstboot."""
> +    """This MixIn class marks Spokes as usable for Firstboot.
> +
> +       Like FirstbootSpokeMixIn, this class should be placed first in the
> +       inheritance order.
> +    """
>      @classmethod
>      def should_run(cls, environment, data):
>          """This method is responsible for beginning Spoke initialization
> diff --git a/pyanaconda/ui/gui/spokes/user.py b/pyanaconda/ui/gui/spokes/user.py
> index 07d9012..88ea50a 100644
> --- a/pyanaconda/ui/gui/spokes/user.py
> +++ b/pyanaconda/ui/gui/spokes/user.py
> @@ -31,8 +31,7 @@ from pyanaconda.ui.common import FirstbootSpokeMixIn
>  from pyanaconda.ui.gui.utils import enlightbox
>  from pyanaconda.ui.helpers import GUISpokeInputCheckHandler, GUIInputCheckHandler, InputCheck
>  
> -from pykickstart.constants import FIRSTBOOT_RECONFIG
> -from pyanaconda.constants import ANACONDA_ENVIRON, FIRSTBOOT_ENVIRON,\
> +from pyanaconda.constants import FIRSTBOOT_ENVIRON,\
>          PASSWORD_EMPTY_ERROR, PASSWORD_CONFIRM_ERROR_GUI, PASSWORD_STRENGTH_DESC,\
>          PASSWORD_WEAK, PASSWORD_WEAK_WITH_ERROR, PASSWORD_WEAK_CONFIRM,\
>          PASSWORD_WEAK_CONFIRM_WITH_ERROR
> @@ -202,17 +201,12 @@ class UserSpoke(FirstbootSpokeMixIn, NormalSpoke, GUISpokeInputCheckHandler):
>  
>      @classmethod
>      def should_run(cls, environment, data):
> -        # the user spoke should run always in the anaconda and in firstboot only
> -        # when doing reconfig or if no user has been created in the installation
> -        if environment == ANACONDA_ENVIRON:
> -            return True
> -        elif environment == FIRSTBOOT_ENVIRON and data is None:
> -            # cannot decide, stay in the game and let another call with data
> -            # available (will come) decide
> +        # Check the superclass should_run
> +        if super(UserSpoke, cls).should_run(environment, data):
>              return True
> +        # If running in firstboot, check if a user has been created
This comment should mention --reconfig. I.e. "If running in firstboot
without --reconfig, check if a user has been created".

>          elif environment == FIRSTBOOT_ENVIRON and data and \
> -                (data.firstboot.firstboot == FIRSTBOOT_RECONFIG or \
> -                     len(data.user.userList) == 0):
> +                len(data.user.userList) == 0:
>              return True
>          else:
>              return False
> diff --git a/pyanaconda/ui/tui/spokes/user.py b/pyanaconda/ui/tui/spokes/user.py
> index 8cc96ac..8636bcf 100644
> --- a/pyanaconda/ui/tui/spokes/user.py
> +++ b/pyanaconda/ui/tui/spokes/user.py
> @@ -24,8 +24,7 @@ from pyanaconda.ui.tui.spokes import EditTUISpokeEntry as Entry
>  from pyanaconda.ui.common import FirstbootSpokeMixIn
>  from pyanaconda.users import guess_username
>  from pyanaconda.i18n import N_, _
> -from pykickstart.constants import FIRSTBOOT_RECONFIG
> -from pyanaconda.constants import ANACONDA_ENVIRON, FIRSTBOOT_ENVIRON
> +from pyanaconda.constants import FIRSTBOOT_ENVIRON
>  from pyanaconda.regexes import GECOS_VALID, USERNAME_VALID, GROUPLIST_SIMPLE_VALID
>  
>  __all__ = ["UserSpoke"]
> @@ -46,23 +45,17 @@ class UserSpoke(FirstbootSpokeMixIn, EditTUISpoke):
>  
>      @classmethod
>      def should_run(cls, environment, data):
> -        # the user spoke should run always in the anaconda and in firstboot only
> -        # when doing reconfig or if no user has been created in the installation
> -        if environment == ANACONDA_ENVIRON:
> -            return True
> -        elif environment == FIRSTBOOT_ENVIRON and data is None:
> -            # cannot decide, stay in the game and let another call with data
> -            # available (will come) decide
> +        # Check the superclass should_run
> +        if super(UserSpoke, cls).should_run(environment, data):
>              return True
> +        # If running in firstboot, check if a user has been recreated
The same applies here.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list