Message-Id: <201606082227.20875@pali>
Date:	Wed, 8 Jun 2016 22:27:20 +0200
From:	Pali Rohár <pali.rohar@...il.com>
To:	Darren Hart <dvhart@...radead.org>
Cc:	Michał Kępień <kernel@...pniu.pl>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Gabriele Mazzotta <gabriele.mzt@...il.com>,
	Mario Limonciello <mario_limonciello@...l.com>,
	Andy Lutomirski <luto@...nel.org>,
	Alex Hung <alex.hung@...onical.com>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] dell-wmi: Sort WMI event codes and update comments

On Wednesday 08 June 2016 22:15:18 Darren Hart wrote:
> On Wed, Jun 08, 2016 at 09:57:26PM +0200, Pali Rohár wrote:
> > On Wednesday 08 June 2016 21:48:24 Darren Hart wrote:
> > > On Wed, Jun 08, 2016 at 12:03:24AM +0200, Pali Rohár wrote:
> > > > On Thursday 02 June 2016 12:41:42 Michał Kępień wrote:
> > > > > > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> > > > > 
> > > > > My guess is that Darren won't let you off without at least a
> > > > > short commit message.
> > > > 
> > > > I have no idea what else to write. I think that description is
> > > > enough.
> > > 
> > > There is always something. For example, why? See
> > > Documentation/SubmittingPatches section "14) The canonical patch
> > > format" for an explanation.
> > > 
> > > "Traceability" of changes is important. If it's worth preparing
> > > the patch, it's worth documenting why.
> > 
> > In my opinion current description is enough and cover everything
> > what this patch is doing. I think it is clear from my description
> > what this patch is doing and so it is documented.
> > 
> > But if it is not clear and something is missing, let me know or
> > show what is wrong and how you change it... It is just my
> > assumption that "Sort WMI event codes and update comments" is
> > clear...
> 
> The patch summary accurately states what it does. It does not explain
> why it does it, or why it is necessary. I understand this is a
> trivial change, but also understand that both maintainers and people
> doing maintenance and regression analysis will appreciate
> understanding the motivation and intent of the patch, in addition to
> the content of the patch.
> 
> From the maintainer perspective, whether we have 20 or 200 patches to
> review, we will naturally merge the ones that require the least
> effort first. This maximizes our efficiency and benefits the most
> people with what time we have available. For many of us, this is our
> nights and weekends (guessing that's the case for you as well). It
> is in the submitter's best interest to take the time document the
> why, what, and how of each patch in a way that minimizes the effort
> on the part of the maintainer to decide if the patch should be
> merged. It is also a matter of scale, if every patch conforms to
> these guidelines, the workflow is much more efficient.
> 
> In this case, I don't know why you decided to sort the event codes or
> update the comments. I assume the comments were wrong before, but
> maybe something changed. Do you care about alphabetically order or
> optimizing the switch statements? Is it purely for legibility? Etc.
> 
> If that isn't sufficient, then just do it out of self-interest,
> because I will not send patches to Linus that do not provide both a
> summary and a description in the commit which meet the guidelines of
> section 14 referenced above.
> 
> Thanks,

I fully understand your maintainer perspective. I just though that my 
one line explain everything what is needed about my patch...

So do you want to include reason for my patch? What about this 
additional description?

===
For better readability of keymap table, sort events by codes and also 
update comments for events to be more informative.
===

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists