Patchwork bundle2: handleoutput i18n

login
register
mail settings
Submitter Akihiko Odaki
Date Aug. 28, 2016, 1:57 a.m.
Message ID <3e9c35285720b3eecbda.1472349441@root3-X220t>
Download mbox | patch
Permalink /patch/16474/
State Changes Requested
Headers show

Comments

Akihiko Odaki - Aug. 28, 2016, 1:57 a.m.
# HG changeset patch
# User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
# Date 1472348629 -32400
#      Sun Aug 28 10:43:49 2016 +0900
# Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
# Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
bundle2: handleoutput i18n
Matt Mackall - Aug. 30, 2016, 12:38 a.m.
On Sun, 2016-08-28 at 10:57 +0900, Akihiko Odaki wrote:
> # HG changeset patch
> # User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
> # Date 1472348629 -32400
> #      Sun Aug 28 10:43:49 2016 +0900
> # Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
> # Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
> bundle2: handleoutput i18n

This would be easier to read as "bundle2: localize handleoutput remote prompts"

> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1467,7 +1467,7 @@ def handlecheckheads(op, inpart):
>  def handleoutput(op, inpart):
>      """forward output captured on the server to the client"""
>      for line in inpart.read().splitlines():
> -        op.ui.status(('remote: %s\n' % line))

You'll note there is an extra level of useless parentheses here. They actually
serve a purpose: they stop check-code.py from complaining about not using _().

So someone didn't put _() there on purpose. Whenever you see something
apparently pointless that was also clearly done on purpose, you need to figure
out why it was done before you take it out: you might be breaking something that
isn't obvious.

But if you look into it and discover there was indeed no good reason for it, you
should explain that in your commit message. Then reviewers will know you were
paying attention.

> +        op.ui.status((_('remote: %s\n') % line))

Leaving in the extra parentheses now useless for real.

-- 
Mathematics is the supreme nostalgia of our time.
Akihiko Odaki - Aug. 30, 2016, 2:10 a.m.
Hi,

Thanl you for reviewing my patch.

On 2016-08-30 09:38, Matt Mackall wrote:
 > On Sun, 2016-08-28 at 10:57 +0900, Akihiko Odaki wrote:
 >> # HG changeset patch
 >> # User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
 >> # Date 1472348629 -32400
 >> #      Sun Aug 28 10:43:49 2016 +0900
 >> # Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
 >> # Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
 >> bundle2: handleoutput i18n
 >
 > This would be easier to read as "bundle2: localize handleoutput 
remote prompts"

I understand.

 >
 >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
 >> --- a/mercurial/bundle2.py
 >> +++ b/mercurial/bundle2.py
 >> @@ -1467,7 +1467,7 @@ def handlecheckheads(op, inpart):
 >>  def handleoutput(op, inpart):
 >>      """forward output captured on the server to the client"""
 >>      for line in inpart.read().splitlines():
 >> -        op.ui.status(('remote: %s\n' % line))
 >
 > You'll note there is an extra level of useless parentheses here. They 
actually
 > serve a purpose: they stop check-code.py from complaining about not 
using _().
 >
 > So someone didn't put _() there on purpose. Whenever you see something
 > apparently pointless that was also clearly done on purpose, you need 
to figure
 > out why it was done before you take it out: you might be breaking 
something that
 > isn't obvious.

I thought it should use _() because other codes do so. Here is `grep -r 
'remote: ' mercurial`.

mercurial/commands.py:            ui.write(_('remote: %s\n') % (', 
'.join(t)))
mercurial/commands.py:            ui.status(_('remote: (synced)\n'))
mercurial/exchange.py:            pushop.ui.status(_('remote: %s\n') % exc)
mercurial/phases.py:                               ' from remote: %s\n') 
% nhex)
mercurial/phases.py:            repo.ui.warn(_('ignoring unexpected root 
from remote: %i %s\n')
mercurial/wireproto.py:            self.ui.status(_('remote: '), l)
mercurial/wireproto.py:                self.ui.status(_('remote: '), l)
mercurial/merge.py:    repo.ui.debug(" ancestor: %s, local: %s, remote: 
%s\n" % (pa, wctx, p2))
mercurial/sshpeer.py:            ui.status(_("remote: "), l, '\n')
mercurial/sshpeer.py:                self.ui.debug("remote: ", l)
mercurial/sshpeer.py:                self.ui.status(_("remote: "), l)
mercurial/bundle2.py:        op.ui.status(('remote: %s\n' % line))

If it is intentional, however, I would like to ask the reason.

 >
 > But if you look into it and discover there was indeed no good reason 
for it, you
 > should explain that in your commit message. Then reviewers will know 
you were
 > paying attention.
 >
 >> +        op.ui.status((_('remote: %s\n') % line))
 >
 > Leaving in the extra parentheses now useless for real.
 >
 > --
 > Mathematics is the supreme nostalgia of our time.
 >

Regards,
Akihiko Odaki
Matt Mackall - Aug. 30, 2016, 4:19 a.m.
On Tue, 2016-08-30 at 11:10 +0900, Akihiko Odaki wrote:
> Hi,
> 
> Thanl you for reviewing my patch.
> 
> On 2016-08-30 09:38, Matt Mackall wrote:
>  > On Sun, 2016-08-28 at 10:57 +0900, Akihiko Odaki wrote:
>  >> # HG changeset patch
>  >> # User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
>  >> # Date 1472348629 -32400
>  >> #      Sun Aug 28 10:43:49 2016 +0900
>  >> # Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
>  >> # Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
>  >> bundle2: handleoutput i18n
>  >
>  > This would be easier to read as "bundle2: localize handleoutput 
> remote prompts"
> 
> I understand.
> 
>  >
>  >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>  >> --- a/mercurial/bundle2.py
>  >> +++ b/mercurial/bundle2.py
>  >> @@ -1467,7 +1467,7 @@ def handlecheckheads(op, inpart):
>  >>  def handleoutput(op, inpart):
>  >>      """forward output captured on the server to the client"""
>  >>      for line in inpart.read().splitlines():
>  >> -        op.ui.status(('remote: %s\n' % line))
>  >
>  > You'll note there is an extra level of useless parentheses here. They 
> actually
>  > serve a purpose: they stop check-code.py from complaining about not 
> using _().
>  >
>  > So someone didn't put _() there on purpose. Whenever you see something
>  > apparently pointless that was also clearly done on purpose, you need 
> to figure
>  > out why it was done before you take it out: you might be breaking 
> something that
>  > isn't obvious.
> 
> I thought it should use _() because other codes do so. Here is `grep -r 
> 'remote: ' mercurial`.

Yes, that does appear right. But did you also check the history of the file
you're changing to see if there was a reason that someone used () to silence a
warning rather than just using _() correctly?

Again: someone went to extra effort to do it "wrong". Before we can "fix" it, we
should try to figure out why.[1]

Also, if you figure out why, don't explain it to me, explain it to your commit
message editor.

[1] This principle is sometimes known as Chesterton's Fence:
https://en.wikipedia.org/wiki/Wikipedia:Chesterton's_fence

-- 
Mathematics is the supreme nostalgia of our time.
Akihiko Odaki - Aug. 31, 2016, 1:57 p.m.
On 2016-08-30 13:19, Matt Mackall wrote:
> On Tue, 2016-08-30 at 11:10 +0900, Akihiko Odaki wrote:
>> Hi,
>>
>> Thanl you for reviewing my patch.
>>
>> On 2016-08-30 09:38, Matt Mackall wrote:
>>  > On Sun, 2016-08-28 at 10:57 +0900, Akihiko Odaki wrote:
>>  >> # HG changeset patch
>>  >> # User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
>>  >> # Date 1472348629 -32400
>>  >> #      Sun Aug 28 10:43:49 2016 +0900
>>  >> # Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
>>  >> # Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
>>  >> bundle2: handleoutput i18n
>>  >
>>  > This would be easier to read as "bundle2: localize handleoutput
>> remote prompts"
>>
>> I understand.
>>
>>  >
>>  >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>>  >> --- a/mercurial/bundle2.py
>>  >> +++ b/mercurial/bundle2.py
>>  >> @@ -1467,7 +1467,7 @@ def handlecheckheads(op, inpart):
>>  >>  def handleoutput(op, inpart):
>>  >>      """forward output captured on the server to the client"""
>>  >>      for line in inpart.read().splitlines():
>>  >> -        op.ui.status(('remote: %s\n' % line))
>>  >
>>  > You'll note there is an extra level of useless parentheses here. They
>> actually
>>  > serve a purpose: they stop check-code.py from complaining about not
>> using _().
>>  >
>>  > So someone didn't put _() there on purpose. Whenever you see something
>>  > apparently pointless that was also clearly done on purpose, you need
>> to figure
>>  > out why it was done before you take it out: you might be breaking
>> something that
>>  > isn't obvious.
>>
>> I thought it should use _() because other codes do so. Here is `grep -r
>> 'remote: ' mercurial`.
>
> Yes, that does appear right. But did you also check the history of the file
> you're changing to see if there was a reason that someone used () to silence a
> warning rather than just using _() correctly?
>

Yes, I checked and it was written by Pierre-Yves David, so he is in Cc. 
The revision is b7435117d951. Sorry for forgetting to tell that.
Matt Mackall - Sept. 1, 2016, 6:15 p.m.
On Wed, 2016-08-31 at 22:57 +0900, Akihiko Odaki wrote:
> On 2016-08-30 13:19, Matt Mackall wrote:
> > 
> > On Tue, 2016-08-30 at 11:10 +0900, Akihiko Odaki wrote:
> > > 
> > > Hi,
> > > 
> > > Thanl you for reviewing my patch.

Since we're cutting a release today, we're going to drop an edited version of
this into the stable branch. Thanks!

-- 
Mathematics is the supreme nostalgia of our time.
Pierre-Yves David - Sept. 6, 2016, 12:20 p.m.
On 08/31/2016 03:57 PM, Akihiko Odaki wrote:
> On 2016-08-30 13:19, Matt Mackall wrote:
>> On Tue, 2016-08-30 at 11:10 +0900, Akihiko Odaki wrote:
>>> Hi,
>>>
>>> Thanl you for reviewing my patch.
>>>
>>> On 2016-08-30 09:38, Matt Mackall wrote:
>>>  > On Sun, 2016-08-28 at 10:57 +0900, Akihiko Odaki wrote:
>>>  >> # HG changeset patch
>>>  >> # User Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
>>>  >> # Date 1472348629 -32400
>>>  >> #      Sun Aug 28 10:43:49 2016 +0900
>>>  >> # Node ID 3e9c35285720b3eecbda43d3d60e483d63ba7ae4
>>>  >> # Parent  4fca69e3d51cc45f331a74caa6e923523ebf7f02
>>>  >> bundle2: handleoutput i18n
>>>  >
>>>  > This would be easier to read as "bundle2: localize handleoutput
>>> remote prompts"
>>>
>>> I understand.
>>>
>>>  >
>>>  >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>>>  >> --- a/mercurial/bundle2.py
>>>  >> +++ b/mercurial/bundle2.py
>>>  >> @@ -1467,7 +1467,7 @@ def handlecheckheads(op, inpart):
>>>  >>  def handleoutput(op, inpart):
>>>  >>      """forward output captured on the server to the client"""
>>>  >>      for line in inpart.read().splitlines():
>>>  >> -        op.ui.status(('remote: %s\n' % line))
>>>  >
>>>  > You'll note there is an extra level of useless parentheses here. They
>>> actually
>>>  > serve a purpose: they stop check-code.py from complaining about not
>>> using _().
>>>  >
>>>  > So someone didn't put _() there on purpose. Whenever you see
>>> something
>>>  > apparently pointless that was also clearly done on purpose, you need
>>> to figure
>>>  > out why it was done before you take it out: you might be breaking
>>> something that
>>>  > isn't obvious.
>>>
>>> I thought it should use _() because other codes do so. Here is `grep -r
>>> 'remote: ' mercurial`.
>>
>> Yes, that does appear right. But did you also check the history of the
>> file
>> you're changing to see if there was a reason that someone used () to
>> silence a
>> warning rather than just using _() correctly?
>>
>
> Yes, I checked and it was written by Pierre-Yves David, so he is in Cc.
> The revision is b7435117d951. Sorry for forgetting to tell that.

I wrote that years ago and I don't remember any details about this 
sorry. If there is no trace in the commit history any reason for this is 
lost.

Cheers,

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1467,7 +1467,7 @@  def handlecheckheads(op, inpart):
 def handleoutput(op, inpart):
     """forward output captured on the server to the client"""
     for line in inpart.read().splitlines():
-        op.ui.status(('remote: %s\n' % line))
+        op.ui.status((_('remote: %s\n') % line))
 
 @parthandler('replycaps')
 def handlereplycaps(op, inpart):