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

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

[Jaben Carsey]

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)
-- 
2.16.2.windows.1

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

[Laszlo Ersek]

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

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

[Carsey, Jaben]

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