Patchwork [3,of,3,STABLE] check-code: detect non bare string inside "_()"

login
register
mail settings
Submitter Katsunori FUJIWARA
Date April 25, 2015, 2:49 p.m.
Message ID <ebfa8410d01987a61a65.1429973352@juju>
Download mbox | patch
Permalink /patch/8790/
State Changes Requested
Headers show

Comments

Katsunori FUJIWARA - April 25, 2015, 2:49 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1429973093 -32400
#      Sat Apr 25 23:44:53 2015 +0900
# Branch stable
# Node ID ebfa8410d01987a61a6548861c272aa5d31560d6
# Parent  ebaf09a1ac2c4e5afd127372cfca611f630b3e36
check-code: detect non bare string inside "_()"

Non bare string inside "_()" causes failure of extracting messages to
be translated: for example, concatenating strings by "+" operator, or
function calling.

This patch adds new check-code pattern to detect non bare string
inside "_()".

"don't use % inside _()" check is also covered by this pattern, and
this patch removes patterns for it.

In addition to it, this patch replaces some intentional "_()"
invocations for non bare string by "_x()", which is alias of "i18n._",
because:

  - adding "no-check-code" may overlook other problems in the future
  - using "i18n.gettext" directly causes unexpected translation in PLAIN mode
Pierre-Yves David - April 25, 2015, 5:10 p.m.
On 04/25/2015 03:49 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1429973093 -32400
> #      Sat Apr 25 23:44:53 2015 +0900
> # Branch stable
> # Node ID ebfa8410d01987a61a6548861c272aa5d31560d6
> # Parent  ebaf09a1ac2c4e5afd127372cfca611f630b3e36
> check-code: detect non bare string inside "_()"

Not super thrilled by that one, there is legitimate use for this.

(more detailled opinions coming later)
Matt Mackall - April 27, 2015, 5:01 p.m.
On Sat, 2015-04-25 at 18:10 +0100, Pierre-Yves David wrote:
> 
> On 04/25/2015 03:49 PM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1429973093 -32400
> > #      Sat Apr 25 23:44:53 2015 +0900
> > # Branch stable
> > # Node ID ebfa8410d01987a61a6548861c272aa5d31560d6
> > # Parent  ebaf09a1ac2c4e5afd127372cfca611f630b3e36
> > check-code: detect non bare string inside "_()"
> 
> Not super thrilled by that one, there is legitimate use for this.

..and he's moved those legitimate users (though I'm not convinced about
all of them!) to use a different interface. Since I've queued dozens and
dozens of patches fixing obvious errors in this area and there are only
a few specialized abusers, I think this patch is a huge win.

However, I'd prefer not to use _x, and instead use i18n.gettext to be
fully explicit. And since it's not actually fixing anything.. it should
wait for the freeze to end.

I've queued the first two patches for stable, thanks.

Patch

diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -212,8 +212,8 @@  pypats = [
     (r'\s<>\s', '<> operator is not available in Python 3+, use !='),
     (r'^\s*\t', "don't use tabs"),
     (r'\S;\s*\n', "semicolon"),
-    (r'[^_]_\([ \t\n]*(?:"[^"]+"[ \t\n+]*)+%', "don't use % inside _()"),
-    (r"[^_]_\([ \t\n]*(?:'[^']+'[ \t\n+]*)+%", "don't use % inside _()"),
+    (r'[^\w]_\([ \t\n]*(?:(?:"[^"]+"|\'[^\']+\')[ \t\n]*)*[^"\' \t\n)]',
+     "put only bare string inside _()"),
     (r'(\w|\)),\w', "missing whitespace after ,"),
     (r'(\w|\))[+/*\-<>]\w', "missing whitespace in expression"),
     (r'^\s+(\w|\.)+=\w[^,()\n]*$', "missing whitespace in assignment"),
diff --git a/hgext/strip.py b/hgext/strip.py
--- a/hgext/strip.py
+++ b/hgext/strip.py
@@ -3,7 +3,7 @@ 
 This extension allows you to strip changesets and all their descendants from the
 repository. See the command help for details.
 """
-from mercurial.i18n import _
+from mercurial.i18n import _, _ as _x
 from mercurial.node import nullid
 from mercurial.lock import release
 from mercurial import cmdutil, hg, scmutil, util
@@ -34,10 +34,10 @@  def checklocalchanges(repo, force=False,
     if not force:
         if s.modified or s.added or s.removed or s.deleted:
             _("local changes found") # i18n tool detection
-            raise util.Abort(_("local changes found" + excsuffix))
+            raise util.Abort(_x("local changes found" + excsuffix))
         if checksubstate(repo):
             _("local changed subrepos found") # i18n tool detection
-            raise util.Abort(_("local changed subrepos found" + excsuffix))
+            raise util.Abort(_x("local changed subrepos found" + excsuffix))
     return s
 
 def strip(ui, repo, revs, update=True, backup=True, force=None, bookmark=None):
diff --git a/mercurial/help.py b/mercurial/help.py
--- a/mercurial/help.py
+++ b/mercurial/help.py
@@ -5,7 +5,7 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
-from i18n import gettext, _
+from i18n import gettext, _, _ as _x
 import itertools, os, textwrap
 import error
 import extensions, revset, fileset, templatekw, templatefilters, filemerge
@@ -101,7 +101,7 @@  def topicmatch(kw):
         else:
             summary = ''
         # translate docs *before* searching there
-        docs = _(getattr(entry[0], '__doc__', None)) or ''
+        docs = _x(getattr(entry[0], '__doc__', None)) or ''
         if kw in cmd or lowercontains(summary) or lowercontains(docs):
             doclines = docs.splitlines()
             if doclines:
diff --git a/mercurial/hgweb/webcommands.py b/mercurial/hgweb/webcommands.py
--- a/mercurial/hgweb/webcommands.py
+++ b/mercurial/hgweb/webcommands.py
@@ -14,7 +14,7 @@  from common import paritygen, staticfile
 from common import HTTP_OK, HTTP_FORBIDDEN, HTTP_NOT_FOUND
 from mercurial import graphmod, patch
 from mercurial import scmutil
-from mercurial.i18n import _
+from mercurial.i18n import _, _ as _x
 from mercurial.error import ParseError, RepoLookupError, Abort
 from mercurial import revset
 
@@ -1256,7 +1256,7 @@  def graph(web, req, tmpl):
 def _getdoc(e):
     doc = e[0].__doc__
     if doc:
-        doc = _(doc).split('\n')[0]
+        doc = _x(doc).split('\n')[0]
     else:
         doc = _('(no help text available)')
     return doc
diff --git a/tests/test-check-code.t b/tests/test-check-code.t
--- a/tests/test-check-code.t
+++ b/tests/test-check-code.t
@@ -270,19 +270,19 @@ 
   $ "$check_code" ./map-inside-gettext.py
   ./map-inside-gettext.py:1:
    > print _("map inside gettext %s" % v)
-   don't use % inside _()
+   put only bare string inside _()
   ./map-inside-gettext.py:3:
    > print _("concatenating " " by " " space %s" % v)
-   don't use % inside _()
+   put only bare string inside _()
   ./map-inside-gettext.py:4:
    > print _("concatenating " + " by " + " '+' %s" % v)
-   don't use % inside _()
+   put only bare string inside _()
   ./map-inside-gettext.py:6:
    > print _("mapping operation in different line %s"
-   don't use % inside _()
+   put only bare string inside _()
   ./map-inside-gettext.py:9:
    > print _(
-   don't use % inside _()
+   put only bare string inside _()
   [1]
 
 web templates