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

Nice patch! See comments below. on 2014/04/08 09:38, shaohef(a)linux.vnet.ibm.com wrote: > From: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; > Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; > --- > 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() >