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

["Zhu, Yonghong" <yonghong.zhu@intel.com>]

It looks good to me.  I will strip the trailing space when push this patch.

Best Regards,
Zhu Yonghong


-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Saturday, May 05, 2018 6:43 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 v3 1/1] BaseTools: Ecc - add dict for config file to internal translation

On 05/04/18 23:47, Carsey, Jaben wrote:
> Thanks.  I don’t push BaseTools, but will await Liming or Yonghong to review and push.

Ah, sure, I didn't mean that you should skip their review; I thought you'd push the patch after their review (you co-maintain several packages, so technically you can push). But, if they push the patch for you, that's fine as well. :)

Thanks!
Laszlo

> 
> Yonghong/Liming,  Can you strip the trailing space before you commit?
> 
> -Jaben
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, May 04, 2018 2:44 PM
>> 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 v3 1/1] BaseTools: Ecc - add dict for config file 
>> to internal translation
>> Importance: High
>>
>> Hi Jaben.
>>
>> On 05/04/18 22:25, Jaben Carsey wrote:
>>> Commit eece4292acc80 changed a variable name, which was tied 
>>> directly to a config file entry. This seperates the internal 
>>> variable names from the config file entries by having the internal 
>>> dict accessed through a translation of key words.
>>>
>>> added a test when this is run straight from command line.
>>>
>>> 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 | 122
>> +++++++++++++++++++-
>>>  1 file changed, 119 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/BaseTools/Source/Python/Ecc/Configuration.py
>> b/BaseTools/Source/Python/Ecc/Configuration.py
>>> index b5b583be8c4a..d305182a2666 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,109 @@ from Common.DataType import *  from 
>>> Common.String import *  from Common.LongFilePathSupport import 
>>> OpenLongFilePath as open
>>>
>>> +_ConfigFileToInternalTranslation = {
>>> +    # not same
>>> +    "ModifierList":"ModifierSet",
>>> +
>>
>> Git complains that the above line contains trailing whitespace.
>>
>> But, that can be removed when you push this patch.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> I've also run the Ecc tool (I didn't try to execute the self-test), 
>> and now it seems to work fine.
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks!
>> Laszlo