Re: [PATCH v1 1/1] BaseTools: Ecc - add dict for config file to internal translation

["Carsey, Jaben" <jaben.carsey@intel.com>]

I worked up and sent out a v2.

I think that if we only use the translation when acing the dict, we can leave the rest of the code as it.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, May 04, 2018 9:20 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong
> <yonghong.zhu@intel.com>
> Subject: Re: [PATCH v1 1/1] BaseTools: Ecc - add dict for config file to internal
> translation
> Importance: High
> 
> On 05/04/18 18:01, Jaben Carsey wrote:
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jaben Carsey <jaben.carsey@intel.com>
> > ---
> >  BaseTools/Source/Python/Ecc/Configuration.py | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Source/Python/Ecc/Configuration.py
> b/BaseTools/Source/Python/Ecc/Configuration.py
> > index b5b583be8c4a..8bc6975c3b41 100644
> > --- a/BaseTools/Source/Python/Ecc/Configuration.py
> > +++ b/BaseTools/Source/Python/Ecc/Configuration.py
> > @@ -1,7 +1,7 @@
> >  ## @file
> >  # This file is used to define class Configuration
> >  #
> > -# Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
> >  # This program and the accompanying materials
> >  # are licensed and made available under the terms and conditions of the
> BSD License
> >  # which accompanies this distribution.  The full text of the license may be
> found at
> > @@ -20,6 +20,10 @@ from Common.DataType import *
> >  from Common.String import *
> >  from Common.LongFilePathSupport import OpenLongFilePath as open
> >
> > +_ConfigFileToInternalTranslation = {
> > +    "ModifierList":"ModifierSet"
> > +    }
> > +
> >  ## Configuration
> >  #
> >  # This class is used to define all items in configuration file
> > @@ -297,6 +301,8 @@ class Configuration(object):
> >              Line = CleanString(Line)
> >              if Line != '':
> >                  List = GetSplitValueList(Line, TAB_EQUAL_SPLIT)
> > +                if List[0] in _ConfigFileToInternalTranslation:
> > +                    List[0] = _ConfigFileToInternalTranslation[List[0]]
> >                  if List[0] not in self.__dict__:
> >                      ErrorMsg = "Invalid configuration option '%s' was found" % List[0]
> >                      EdkLogger.error("Ecc", EdkLogger.ECC_ERROR, ErrorMsg, File =
> Filepath, Line = LineNo)
> >
> 
> I have some comments:
> 
> (1) Can you please give a short summary in the commit message why this
> patch is needed?
> 
> I suggest we also reference the commit that introduced the problem:
> eece4292acc80.
> 
> (2) I think at this point we have only a handful of keywords / config
> options that we recognize. Is that right? Because, I'd like to suggest
> the following:
> 
> - add them all to the dictionary (and they all will be identity-mapped
>   except for "ModifierList":"ModifierSet"),
> - always go through the dict; don't make it optional.
> 
> Reasons:
> - if someone introduces a new option, they won't be able to parse it
> without adding it first to the dict as well (with identity mapping),
> - and keeping everything in the dict all the time is good because if
> someone renames something else next time, their text seach will
> immediately turn up the mapping as well, and they won't forget to update
> it right off the bat. It's easier to update an occurrence that your
> search finds than remembering to add a new translation.
> 
> (3) In connection with (2), I'm not really a fan of translating List[0]
> in-place. I think we should have a separate variable for original and
> translated name.
> 
> ... In fact, I think this patch is technically incorrect; it translates
> ModifierList to ModifierSet in-place, which might be good for accessing
> the renamed object member; however, the option check still -- correctly
> -- uses the public option name:
> 
>         if List[0] == 'ModifierList':
>             List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT)
> 
> and now it wouldn't match.
> 
> So even if we do translate List[0] in-place, we'll have to preserve the
> original option name that was read from the file, for option matching.
> 
> Thanks
> Laszlo