Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

From: Pratyush Yadav <me@yadavpratyush.com>
To: Birger Skogeng Pedersen <birger.sp@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus
Date: Tue, 3 Sep 2019 00:23:21 +0530	[thread overview]
Message-ID: <20190902185321.m76dffzfc76astar@yadavpratyush.com> (raw)
In-Reply-To: <CAGr--=L64jiE=x_McdfWJuQTz1Qo3wp0nc8xaeKBGe0GgW5Kxg@mail.gmail.com>

On 02/09/19 08:35PM, Birger Skogeng Pedersen wrote:
> On Mon, Sep 2, 2019 at 8:19 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > You missed removing this extra blank line. Will fix it up locally.
> >
> > > +             set _index $file_lists_last_clicked($widget)
> > [snip]
> 
> 
> To be honest I felt the blank line there was appropriate, in an
> attempt to follow the same code style as the rest of the git-gui.sh.
> For instance, the "add_range_to_selection" function
> (https://github.com/prati0100/git-gui/blob/master/git-gui.sh#L2601-L2625)
> has blank lines like this;

Oh yes, these types of blank lines are perfectly fine in my opinion too.  
I actually prefer this alternative over not having any spacing at all.  
This way you divide the function in logical sections/steps.

But that was not what I pointed out. The blank line I pointed out was 
like this:

  if {condition} {

	foo
	bar
  }

My nitpick was with the blank line _just after_ the if.

> 
> proc add_range_to_selection {w x y} {
>     global file_lists last_clicked selected_paths
> 

So in this example, the above blank line is fine...

>     if {[lindex $last_clicked 0] ne $w} {

...but if I add a blank line here like:

  if {[lindex $last_clicked 0] ne $w} {

	toggle_or_diff click $w $x $y
	return
  }

instead of like:

  if {[lindex $last_clicked 0] ne $w} {
	toggle_or_diff click $w $x $y
	return
  }

It is the blank line just after the start of your if statement that I 
was pointing out. I did leave other blank lines untouched in your patch.

[snip]

-- 
Regards,
Pratyush Yadav