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

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

Incoming v3.  I found that some more entries in the dict were missing due to config.ini sample file vs class membership. Not everything was in the sample file.  I also decided to add a test function to use to verify class and dict completeness.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, May 04, 2018 11:25 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 v2 1/1] BaseTools: Ecc - add dict for config file to internal
> translation
> Importance: High
> 
> On 05/04/18 18:46, Jaben Carsey wrote:
> > Commit eece4292acc80 changed a variable name, which was tied directly to
> a
> > config file entry.  this seperates the itnernal variable names from the
> > config file entries by having the internal dict accessed through a translation
> > of key words.
> >
> > 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 | 101
> +++++++++++++++++++-
> >  1 file changed, 98 insertions(+), 3 deletions(-)
> >
> > diff --git a/BaseTools/Source/Python/Ecc/Configuration.py
> b/BaseTools/Source/Python/Ecc/Configuration.py
> > index b5b583be8c4a..72377070f831 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,101 @@ from Common.DataType import *
> >  from Common.String import *
> >  from Common.LongFilePathSupport import OpenLongFilePath as open
> >
> > +_ConfigFileToInternalTranslation = {
> > +    # not same
> > +    "ModifierList":"ModifierSet",
> > +
> > +    # same
> > +    "Version":"Version",
> > +    "CheckAll":"CheckAll",
> > +    "AutoCorrect":"AutoCorrect",
> > +    "GeneralCheckAll":"GeneralCheckAll",
> > +    "GeneralCheckNoTab":"GeneralCheckNoTab",
> > +    "GeneralCheckTabWidth":"GeneralCheckTabWidth",
> > +    "GeneralCheckIndentation":"GeneralCheckIndentation",
> > +    "GeneralCheckIndentationWidth":"GeneralCheckIndentationWidth",
> > +    "GeneralCheckLine":"GeneralCheckLine",
> > +    "GeneralCheckLineWidth":"GeneralCheckLineWidth",
> > +    "GeneralCheckNo_Asm":"GeneralCheckNo_Asm",
> > +    "GeneralCheckNoProgma":"GeneralCheckNoProgma",
> > +    "GeneralCheckCarriageReturn":"GeneralCheckCarriageReturn",
> > +    "GeneralCheckFileExistence":"GeneralCheckFileExistence",
> > +    "GeneralCheckNonAcsii":"GeneralCheckNonAcsii",
> > +    "GeneralCheckUni":"GeneralCheckUni",
> > +    "SpaceCheckAll":"SpaceCheckAll",
> > +    "PredicateExpressionCheckAll":"PredicateExpressionCheckAll",
> > +
> "PredicateExpressionCheckBooleanValue":"PredicateExpressionCheckBoole
> anValue",
> > +
> "PredicateExpressionCheckNonBooleanOperator":"PredicateExpressionChec
> kNonBooleanOperator",
> > +
> "PredicateExpressionCheckComparisonNullType":"PredicateExpressionChec
> kComparisonNullType",
> > +    "HeaderCheckAll":"HeaderCheckAll",
> > +    "HeaderCheckFile":"HeaderCheckFile",
> > +    "HeaderCheckFunction":"HeaderCheckFunction",
> > +    "HeaderCheckFileCommentEnd":"HeaderCheckFileCommentEnd",
> > +
> "HeaderCheckCFileCommentStartSpacesNum":"HeaderCheckCFileComment
> StartSpacesNum",
> > +
> "HeaderCheckCFileCommentReferenceFormat":"HeaderCheckCFileCommen
> tReferenceFormat",
> > +
> "HeaderCheckCFileCommentLicenseFormat":"HeaderCheckCFileCommentLic
> enseFormat",
> > +    "CFunctionLayoutCheckAll":"CFunctionLayoutCheckAll",
> > +
> "CFunctionLayoutCheckReturnType":"CFunctionLayoutCheckReturnType",
> > +
> "CFunctionLayoutCheckOptionalFunctionalModifier":"CFunctionLayoutCheck
> OptionalFunctionalModifier",
> > +
> "CFunctionLayoutCheckFunctionName":"CFunctionLayoutCheckFunctionNa
> me",
> > +
> "CFunctionLayoutCheckFunctionPrototype":"CFunctionLayoutCheckFunction
> Prototype",
> > +
> "CFunctionLayoutCheckFunctionBody":"CFunctionLayoutCheckFunctionBody
> ",
> > +
> "CFunctionLayoutCheckDataDeclaration":"CFunctionLayoutCheckDataDeclara
> tion",
> > +
> "CFunctionLayoutCheckNoInitOfVariable":"CFunctionLayoutCheckNoInitOfV
> ariable",
> > +    "CFunctionLayoutCheckNoStatic":"CFunctionLayoutCheckNoStatic",
> > +    "IncludeFileCheckAll":"IncludeFileCheckAll",
> > +    "IncludeFileCheckSameName":"IncludeFileCheckSameName",
> > +
> "IncludeFileCheckIfndefStatement":"IncludeFileCheckIfndefStatement",
> > +    "IncludeFileCheckData":"IncludeFileCheckData",
> > +    "DeclarationDataTypeCheckAll":"DeclarationDataTypeCheckAll",
> > +
> "DeclarationDataTypeCheckNoUseCType":"DeclarationDataTypeCheckNoUs
> eCType",
> > +
> "DeclarationDataTypeCheckInOutModifier":"DeclarationDataTypeCheckInOu
> tModifier",
> > +
> "DeclarationDataTypeCheckEFIAPIModifier":"DeclarationDataTypeCheckEFIA
> PIModifier",
> > +
> "DeclarationDataTypeCheckEnumeratedType":"DeclarationDataTypeCheckE
> numeratedType",
> > +
> "DeclarationDataTypeCheckStructureDeclaration":"DeclarationDataTypeChec
> kStructureDeclaration",
> > +
> "DeclarationDataTypeCheckSameStructure":"DeclarationDataTypeCheckSam
> eStructure",
> > +
> "DeclarationDataTypeCheckUnionType":"DeclarationDataTypeCheckUnionTy
> pe",
> > +    "NamingConventionCheckAll":"NamingConventionCheckAll",
> > +
> "NamingConventionCheckDefineStatement":"NamingConventionCheckDefi
> neStatement",
> > +
> "NamingConventionCheckTypedefStatement":"NamingConventionCheckTyp
> edefStatement",
> > +
> "NamingConventionCheckIfndefStatement":"NamingConventionCheckIfnde
> fStatement",
> > +
> "NamingConventionCheckPathName":"NamingConventionCheckPathName"
> ,
> > +
> "NamingConventionCheckVariableName":"NamingConventionCheckVariable
> Name",
> > +
> "NamingConventionCheckFunctionName":"NamingConventionCheckFunctio
> nName",
> > +
> "NamingConventionCheckSingleCharacterVariable":"NamingConventionChec
> kSingleCharacterVariable",
> > +    "DoxygenCheckAll":"DoxygenCheckAll",
> > +    "DoxygenCheckFileHeader":"DoxygenCheckFileHeader",
> > +    "DoxygenCheckFunctionHeader":"DoxygenCheckFunctionHeader",
> > +
> "DoxygenCheckCommentDescription":"DoxygenCheckCommentDescription"
> ,
> > +    "DoxygenCheckCommentFormat":"DoxygenCheckCommentFormat",
> > +    "DoxygenCheckCommand":"DoxygenCheckCommand",
> > +    "MetaDataFileCheckAll":"MetaDataFileCheckAll",
> > +    "MetaDataFileCheckPathName":"MetaDataFileCheckPathName",
> > +
> "MetaDataFileCheckGenerateFileList":"MetaDataFileCheckGenerateFileList",
> > +
> "MetaDataFileCheckPathOfGenerateFileList":"MetaDataFileCheckPathOfGe
> nerateFileList",
> > +
> "MetaDataFileCheckLibraryInstance":"MetaDataFileCheckLibraryInstance",
> > +
> "MetaDataFileCheckLibraryInstanceDependent":"MetaDataFileCheckLibraryI
> nstanceDependent",
> > +
> "MetaDataFileCheckLibraryInstanceOrder":"MetaDataFileCheckLibraryInstan
> ceOrder",
> > +    "MetaDataFileCheckLibraryNoUse":"MetaDataFileCheckLibraryNoUse",
> > +
> "MetaDataFileCheckLibraryDefinedInDec":"MetaDataFileCheckLibraryDefine
> dInDec",
> > +
> "MetaDataFileCheckBinaryInfInFdf":"MetaDataFileCheckBinaryInfInFdf",
> > +    "MetaDataFileCheckPcdDuplicate":"MetaDataFileCheckPcdDuplicate",
> > +    "MetaDataFileCheckPcdFlash":"MetaDataFileCheckPcdFlash",
> > +    "MetaDataFileCheckPcdNoUse":"MetaDataFileCheckPcdNoUse",
> > +
> "MetaDataFileCheckGuidDuplicate":"MetaDataFileCheckGuidDuplicate",
> > +
> "MetaDataFileCheckModuleFileNoUse":"MetaDataFileCheckModuleFileNoU
> se",
> > +    "MetaDataFileCheckPcdType":"MetaDataFileCheckPcdType",
> > +
> "MetaDataFileCheckModuleFileGuidDuplication":"MetaDataFileCheckModul
> eFileGuidDuplication",
> > +    "UniCheckAll":"UniCheckAll",
> > +    "UniCheckHelpInfo":"UniCheckHelpInfo",
> > +    "UniCheckPCDInfo":"UniCheckPCDInfo",
> > +    "GeneralCheckUni":"GeneralCheckUni",
> > +    "SmmCommParaCheckAll":"SmmCommParaCheckAll",
> > +
> "SmmCommParaCheckBufferType":"SmmCommParaCheckBufferType",
> > +    "BinaryExtList":"BinaryExtList",
> > +    "ScanOnlyDirList":"ScanOnlyDirList"
> > +    }
> > +
> >  ## Configuration
> >  #
> >  # This class is used to define all items in configuration file
> > @@ -297,7 +392,7 @@ class Configuration(object):
> >              Line = CleanString(Line)
> >              if Line != '':
> >                  List = GetSplitValueList(Line, TAB_EQUAL_SPLIT)
> > -                if List[0] not in self.__dict__:
> > +                if _ConfigFileToInternalTranslation[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)
> >                  if List[0] == 'ModifierList':
> > @@ -312,7 +407,7 @@ class Configuration(object):
> >                      List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT)
> >                  if List[0] == 'Copyright':
> >                      List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT)
> > -                self.__dict__[List[0]] = List[1]
> > +                self.__dict__[_ConfigFileToInternalTranslation[List[0]]] = List[1]
> >
> >      def ShowMe(self):
> >          print self.Filename
> >
> 
> 
> Ouch, I didn't think the dictionary was this large. :/
> 
> Anyway, I still think this is the right approach. Three comments:
> 
> * The commit message is too wide. It should be 74 chars or so.
> 
> * Can we keep the list sorted (in the source code)? Because, at the
>   moment, the "GeneralCheckUni":"GeneralCheckUni" entry is duplicated.
>   That's easier to notice if the entries are sorted.
> 
>   Maybe add a comment too about keeping the lists sorted.
> 
> * If the config file contains an error (typo or completely bogus option
>   name), now we will not get a nice error message. Because, before we
>   get to evaluate "translated_name in self.__dict__", the translation
>   lookup will itself fail. I think that throws a KeyError exception.
> 
>   I think we should:
>   - report "invalid config option" if the explicit translation fails,
>   - use an additional assertion that the translated option name is in
>     self.__dict__ (it should be an assertion because if it fails, it
>     indicates a bug in Ecc).
> 
> To save you some time, I'll paste the uniquely sorted list of "same"
> entries at the end (87 key:value pairs).
> 
> Thanks!
> Laszlo
> 
>     "AutoCorrect":"AutoCorrect",
>     "BinaryExtList":"BinaryExtList",
>     "CFunctionLayoutCheckAll":"CFunctionLayoutCheckAll",
> 
> "CFunctionLayoutCheckDataDeclaration":"CFunctionLayoutCheckDataDeclara
> tion",
> 
> "CFunctionLayoutCheckFunctionBody":"CFunctionLayoutCheckFunctionBody
> ",
> 
> "CFunctionLayoutCheckFunctionName":"CFunctionLayoutCheckFunctionNa
> me",
> 
> "CFunctionLayoutCheckFunctionPrototype":"CFunctionLayoutCheckFunction
> Prototype",
> 
> "CFunctionLayoutCheckNoInitOfVariable":"CFunctionLayoutCheckNoInitOfV
> ariable",
>     "CFunctionLayoutCheckNoStatic":"CFunctionLayoutCheckNoStatic",
> 
> "CFunctionLayoutCheckOptionalFunctionalModifier":"CFunctionLayoutCheck
> OptionalFunctionalModifier",
>     "CFunctionLayoutCheckReturnType":"CFunctionLayoutCheckReturnType",
>     "CheckAll":"CheckAll",
>     "DeclarationDataTypeCheckAll":"DeclarationDataTypeCheckAll",
> 
> "DeclarationDataTypeCheckEFIAPIModifier":"DeclarationDataTypeCheckEFIA
> PIModifier",
> 
> "DeclarationDataTypeCheckEnumeratedType":"DeclarationDataTypeCheckE
> numeratedType",
> 
> "DeclarationDataTypeCheckInOutModifier":"DeclarationDataTypeCheckInOu
> tModifier",
> 
> "DeclarationDataTypeCheckNoUseCType":"DeclarationDataTypeCheckNoUs
> eCType",
> 
> "DeclarationDataTypeCheckSameStructure":"DeclarationDataTypeCheckSam
> eStructure",
> 
> "DeclarationDataTypeCheckStructureDeclaration":"DeclarationDataTypeChec
> kStructureDeclaration",
> 
> "DeclarationDataTypeCheckUnionType":"DeclarationDataTypeCheckUnionTy
> pe",
>     "DoxygenCheckAll":"DoxygenCheckAll",
>     "DoxygenCheckCommand":"DoxygenCheckCommand",
> 
> "DoxygenCheckCommentDescription":"DoxygenCheckCommentDescription"
> ,
>     "DoxygenCheckCommentFormat":"DoxygenCheckCommentFormat",
>     "DoxygenCheckFileHeader":"DoxygenCheckFileHeader",
>     "DoxygenCheckFunctionHeader":"DoxygenCheckFunctionHeader",
>     "GeneralCheckAll":"GeneralCheckAll",
>     "GeneralCheckCarriageReturn":"GeneralCheckCarriageReturn",
>     "GeneralCheckFileExistence":"GeneralCheckFileExistence",
>     "GeneralCheckIndentation":"GeneralCheckIndentation",
>     "GeneralCheckIndentationWidth":"GeneralCheckIndentationWidth",
>     "GeneralCheckLine":"GeneralCheckLine",
>     "GeneralCheckLineWidth":"GeneralCheckLineWidth",
>     "GeneralCheckNoProgma":"GeneralCheckNoProgma",
>     "GeneralCheckNoTab":"GeneralCheckNoTab",
>     "GeneralCheckNo_Asm":"GeneralCheckNo_Asm",
>     "GeneralCheckNonAcsii":"GeneralCheckNonAcsii",
>     "GeneralCheckTabWidth":"GeneralCheckTabWidth",
>     "GeneralCheckUni":"GeneralCheckUni",
>     "HeaderCheckAll":"HeaderCheckAll",
> 
> "HeaderCheckCFileCommentLicenseFormat":"HeaderCheckCFileCommentLic
> enseFormat",
> 
> "HeaderCheckCFileCommentReferenceFormat":"HeaderCheckCFileCommen
> tReferenceFormat",
> 
> "HeaderCheckCFileCommentStartSpacesNum":"HeaderCheckCFileComment
> StartSpacesNum",
>     "HeaderCheckFile":"HeaderCheckFile",
>     "HeaderCheckFileCommentEnd":"HeaderCheckFileCommentEnd",
>     "HeaderCheckFunction":"HeaderCheckFunction",
>     "IncludeFileCheckAll":"IncludeFileCheckAll",
>     "IncludeFileCheckData":"IncludeFileCheckData",
>     "IncludeFileCheckIfndefStatement":"IncludeFileCheckIfndefStatement",
>     "IncludeFileCheckSameName":"IncludeFileCheckSameName",
>     "MetaDataFileCheckAll":"MetaDataFileCheckAll",
>     "MetaDataFileCheckBinaryInfInFdf":"MetaDataFileCheckBinaryInfInFdf",
> 
> "MetaDataFileCheckGenerateFileList":"MetaDataFileCheckGenerateFileList",
>     "MetaDataFileCheckGuidDuplicate":"MetaDataFileCheckGuidDuplicate",
> 
> "MetaDataFileCheckLibraryDefinedInDec":"MetaDataFileCheckLibraryDefine
> dInDec",
>     "MetaDataFileCheckLibraryInstance":"MetaDataFileCheckLibraryInstance",
> 
> "MetaDataFileCheckLibraryInstanceDependent":"MetaDataFileCheckLibraryI
> nstanceDependent",
> 
> "MetaDataFileCheckLibraryInstanceOrder":"MetaDataFileCheckLibraryInstan
> ceOrder",
>     "MetaDataFileCheckLibraryNoUse":"MetaDataFileCheckLibraryNoUse",
> 
> "MetaDataFileCheckModuleFileGuidDuplication":"MetaDataFileCheckModul
> eFileGuidDuplication",
> 
> "MetaDataFileCheckModuleFileNoUse":"MetaDataFileCheckModuleFileNoU
> se",
>     "MetaDataFileCheckPathName":"MetaDataFileCheckPathName",
> 
> "MetaDataFileCheckPathOfGenerateFileList":"MetaDataFileCheckPathOfGe
> nerateFileList",
>     "MetaDataFileCheckPcdDuplicate":"MetaDataFileCheckPcdDuplicate",
>     "MetaDataFileCheckPcdFlash":"MetaDataFileCheckPcdFlash",
>     "MetaDataFileCheckPcdNoUse":"MetaDataFileCheckPcdNoUse",
>     "MetaDataFileCheckPcdType":"MetaDataFileCheckPcdType",
>     "NamingConventionCheckAll":"NamingConventionCheckAll",
> 
> "NamingConventionCheckDefineStatement":"NamingConventionCheckDefi
> neStatement",
> 
> "NamingConventionCheckFunctionName":"NamingConventionCheckFunctio
> nName",
> 
> "NamingConventionCheckIfndefStatement":"NamingConventionCheckIfnde
> fStatement",
> 
> "NamingConventionCheckPathName":"NamingConventionCheckPathName"
> ,
> 
> "NamingConventionCheckSingleCharacterVariable":"NamingConventionChec
> kSingleCharacterVariable",
> 
> "NamingConventionCheckTypedefStatement":"NamingConventionCheckTyp
> edefStatement",
> 
> "NamingConventionCheckVariableName":"NamingConventionCheckVariable
> Name",
>     "PredicateExpressionCheckAll":"PredicateExpressionCheckAll",
> 
> "PredicateExpressionCheckBooleanValue":"PredicateExpressionCheckBoole
> anValue",
> 
> "PredicateExpressionCheckComparisonNullType":"PredicateExpressionChec
> kComparisonNullType",
> 
> "PredicateExpressionCheckNonBooleanOperator":"PredicateExpressionChec
> kNonBooleanOperator",
>     "ScanOnlyDirList":"ScanOnlyDirList",
>     "SmmCommParaCheckAll":"SmmCommParaCheckAll",
>     "SmmCommParaCheckBufferType":"SmmCommParaCheckBufferType",
>     "SpaceCheckAll":"SpaceCheckAll",
>     "UniCheckAll":"UniCheckAll",
>     "UniCheckHelpInfo":"UniCheckHelpInfo",
>     "UniCheckPCDInfo":"UniCheckPCDInfo",
>     "Version":"Version"