[Kimchi-devel] [PATCH V4] add a make check-local command to verify the i18n string formatting

[Sheldon]

On 04/08/2014 05:50 PM, Zhou Zheng Sheng wrote:
> Nice patch! See comments below.
>
> on 2014/04/08 09:38, shaohef at linux.vnet.ibm.com wrote:
>> From: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>
>> When I do i18n translation, I find some string formattings are wrong.
>> So I add a check-local command to help the developer to check their
>> string formatting.
>>
>> Every developers please run this command before submit your patch.
>>
>> After you run this command, it may report some invalid string
>> formattings as follow.
>> $ make check-local
>> Checking for invalid i18n string...
>> bad i18n string formatting:
>>    KCHAPI0005E: Create is not allowed for %(resource).
>> make: *** [check-local] Error 1
>>
>> You should check %(resource) is what you want.
>>
>> Ref:
>> https://docs.python.org/2/library/stdtypes.html#string-formatting-operations
>>
>> Signed-off-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>> Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
>> ---
>>   Makefile.am           |  5 +++++
>>   contrib/check_i18n.py | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 66 insertions(+)
>>   create mode 100755 contrib/check_i18n.py
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index c68f050..401ff37 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -73,7 +73,12 @@ PEP8_WHITELIST = \
>>
>>   SKIP_PYFLAKES_ERR = "\./src/kimchi/websocket\.py"
>>
>> +I18N_FILES = plugins/*/i18n.py \
>> +	src/kimchi/i18n.py \
>> +	$(NULL)
>> +
>>   check-local:
>> +	contrib/check_i18n.py $(I18N_FILES)
>>   	find . -path './.git' -prune -type f -o \
>>   		-name '*.py' -o -name '*.py.in'  | xargs $(PYFLAKES) | \
>>   		grep -w -v $(SKIP_PYFLAKES_ERR) | \
>> diff --git a/contrib/check_i18n.py b/contrib/check_i18n.py
>> new file mode 100755
>> index 0000000..e3c2b8c
>> --- /dev/null
>> +++ b/contrib/check_i18n.py
>> @@ -0,0 +1,61 @@
>> +#!/usr/bin/python
>> +#
>> +# Project Kimchi
>> +#
>> +# Copyright IBM, Corp. 2014
>> +#
>> +# This library is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU Lesser General Public
>> +# License as published by the Free Software Foundation; either
>> +# version 2.1 of the License, or (at your option) any later version.
>> +#
>> +# This library is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +# Lesser General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU Lesser General Public
>> +# License along with this library; if not, write to the Free Software
>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
>> +
>> +import glob
>> +import imp
>> +import os
>> +import re
>> +import sys
>> +
>> +
>> +PATTERN = re.compile("%\([^\)]*\)[#0\-\+\s]?(\d+|\*)?(\.(\d+|\*)?)?"
>> +                     "[lLh]?[cdeEfFgGioursxX%]")
> It seems the PATTERN does not fully follow the Python specification. In
> "5.6.2. String Formatting Operations", there is a "Flag" table. It shows
> that "#" "0" "-" " "(a space) and "+" are valid flags. It implies the
> following
>    '%(a) d' % {'a': 100}   produces    ' 100'
>    '%(a) d' % {'a': -100}   produces    '-100'
> So the a space should be added to "[#0\-\+\s]" part of the pattern.
yes. Will remove "\s" add " ". check "\s" include " " and "\t".
>
> According to the description of the "5. Precision", it seem that
> "(\.(\d+|\*)?)?" should be changed to "(\.(\d+|\*))?".
yes. it should be "(\.(\d+|\*))?"
>
>> +BAD_PATTERN = re.compile("%\([^\)]*\)")
>> +
>> +
>> +def load_i18n_module(i18nfile):
>> +    path = os.path.dirname(i18nfile)
>> +    mname = i18nfile.replace("/", "_").rstrip(".py")
>> +    mobj = imp.find_module("i18n", [path])
>> +    return imp.load_module(mname, *mobj)
>> +
>> +
>> +def check_string_formatting(messages):
>> +    for k, v in messages.iteritems():
>> +        if BAD_PATTERN.findall(PATTERN.sub(" ", v)):
>> +            print "bad i18n string formatting:"
>> +            print "  %s: %s" % (k, v)
>> +            exit(1)
>> +
>> +
>> +def main():
>> +    files = []
>> +    for v in sys.argv[1:]:
>> +        files.extend(glob.glob(v))
>> +
> It seems that you don't have to do path pattern matching here. The shell
> shall perform the path expansion automatically. When in Makefile it runs
> the following,
>    contrib/check_i18n.py $(I18N_FILES)
> Then make expands I18N_FILES into
>    plugins/*/i18n.py src/kimchi/i18n.py
> Then it runs
>    contrib/check_i18n.py plugins/*/i18n.py src/kimchi/i18n.py
> Then shell expands plugins/*/i18n.py properly. So we don't have to call
> gloab ourselves.
yes. Thanks.
>> +    print "Checking for invalid i18n string..."
>> +    for f in files:
>> +        messages = load_i18n_module(f).messages
>> +        check_string_formatting(messages)
>> +    print "Checking for invalid i18n string successfully"
>> +
>> +
>> +if __name__ == '__main__':
>> +    main()
>>
>


-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef at linux.vnet.ibm.com>
IBM Linux Technology Center