Re: [wic patch 5/5] wic: Use enum like dicts for string constants

[Ed Bartosh <ed.bartosh@linux.intel.com>]

On Wed, May 03, 2017 at 10:47:45AM +0200, Andreas Reichel wrote:
> On Tue, May 02, 2017 at 04:36:22PM +0300, Ed Bartosh wrote:
> > On Fri, Apr 21, 2017 at 02:11:45PM +0200, Andreas J. Reichel wrote:
> > > To increase code maintainability, use dictionaries
> > > as enum-like container for parameter string comparisons.
> > > 
> > > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> > > 
> > > ---
> > >  scripts/lib/wic/engine.py | 16 +++++++++++++---
> > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/scripts/lib/wic/engine.py b/scripts/lib/wic/engine.py
> > > index 647358287f..1428a73ba8 100644
> > > --- a/scripts/lib/wic/engine.py
> > > +++ b/scripts/lib/wic/engine.py
> > > @@ -35,6 +35,16 @@ from wic import WicError
> > >  from wic.pluginbase import PluginMgr
> > >  from wic.utils.misc import get_bitbake_var
> > >  
> > > +class StrEnum(dict):
> > > +    __getattr__ = dict.get
> > > +
> > > +ListType = StrEnum({
> > > +    'LIST_IMAGES': "images",
> > > +    'LIST_SRC_PLUGINS': "source-plugins" })
> > > +
> > > +HelpArg = StrEnum({
> > > +    'HELP': "help" })
> > > +
> > >  logger = logging.getLogger('wic')
> > >  
> > >  def verify_build_env():
> > > @@ -204,14 +214,14 @@ def wic_list(args, scripts_path):
> > >      if args.list_type is None:
> > >          return False
> > >  
> > > -    if args.list_type == "images":
> > > +    if args.list_type == ListType.LIST_IMAGES:
> > >  
> > >          list_canned_images(scripts_path)
> > >          return True
> > > -    elif args.list_type == "source-plugins":
> > > +    elif args.list_type == ListType.LIST_SRC_PLUGINS:
> > >          list_source_plugins()
> > >          return True
> > > -    elif len(args.help_for) == 1 and args.help_for[0] == 'help':
> > > +    elif len(args.help_for) == 1 and args.help_for[0] == HelpArg.HELP:
> > >          wks_file = args.list_type
> > >          fullpath = find_canned_image(scripts_path, wks_file)
> > >          if not fullpath:
> > 
> > I'm not sure if this increases maintainability, but it definitely increases code complexity and
> > decreases readability. Can you explain the idea in a bit more detailed way?
> > 
> In general, if string constants are used as keys, it is better do define them as
> constants. Maintainability increases because if you have to change them,
> you only change them in one place. You have a central place for this
> definition and not scattered them all over the code. That's the idea.
>

Not sure I'm convinced. From my point of view this patch changes quite
simple and readable code to something more complex and less readable for
no visible reason.

If I have to change 'images' or 'source-plugins'(why would I want to do that?)
I'd just go and change them in one place as they're not used anywhere else.

--
Regards,
Ed