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

[Zhou Zheng Sheng]

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.

According to the description of the "5. Precision", it seem that
"(\.(\d+|\*)?)?" should be changed to "(\.(\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.

> +    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!

Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397