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:ler...@redhat.com] > Sent: Friday, May 04, 2018 9:20 AM > To: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel@lists.01.org > Cc: Gao, Liming <liming....@intel.com>; Zhu, Yonghong > <yonghong....@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....@intel.com> > > Cc: Yonghong Zhu <yonghong....@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jaben Carsey <jaben.car...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
