| 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
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.
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
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.
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.
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.
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):