Re: [ovs-dev] [PATCH] CONTRIBUTING.rst: Update patch summary and description style guidelines.

[Ben Pfaff]

On Fri, Dec 02, 2016 at 11:49:03AM +, Stephen Finucane wrote:
> Looks good :)
> 
> Acked-by: Stephen Finucane 

Thanks, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] CONTRIBUTING.rst: Update patch summary and description style guidelines.

[Stephen Finucane]

On Thu, 2016-12-01 at 15:59 -0800, Ben Pfaff wrote:
> On Thu, Dec 01, 2016 at 01:16:14PM +, Stephen Finucane wrote:
> > On Wed, 2016-11-30 at 12:40 -0800, Ben Pfaff wrote:
> > > Suggested-by: Joe Stringer 
> > > Suggested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2016
> > > -Nov
> > > ember/325513.html
> > > Signed-off-by: Ben Pfaff 
> > > ---
> > >  CONTRIBUTING.rst | 12 ++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> > > index 867562e..721371e 100644
> > > --- a/CONTRIBUTING.rst
> > > +++ b/CONTRIBUTING.rst
> > > @@ -94,7 +94,11 @@ Where:
> > >    multiple distinct pieces of code.
> > >  
> > >  :
> > > -  briefly describes the change.
> > > +
> > > +  briefly describes the change.  Use the the imperative form,
> > > +  e.g. "Force SNAT for multiple gateway routers." or "Fix daemon
> > > exit
> > > +  for bad datapaths or flows."  This turns the summary into a
> > > +  sentence, so please end it with a period.
> > 
> > This is super nitty (sorry), but the full stop takes up 1/50th of
> > the
> > available summary line characters [1] without adding any value :)
> > I'd
> > argue that its use certainly should be, at best, optional. If we're
> > adding anything extra, I'd recommend that summary lines should be
> > constrained to 50/51 characters where possible (vim does this
> > itself).
> 
> Perhaps the period is more of a personal preference.  I'll drop it.
> 
> I find 50 characters to be almost impossible in many cases, myself,
> but
> we can at least recommend it as something to try for.
> 
> > >  The subject, minus the ``[PATCH /]`` prefix, becomes the
> > > first
> > > line of
> > >  the commit's change log message.
> > > @@ -106,7 +110,9 @@ The body of the email should start with a
> > > more
> > > thorough description of the
> > >  change.  This becomes the body of the commit message, following
> > > the
> > > subject.
> > >  There is no need to duplicate the summary given in the subject.
> > >  
> > > -Please limit lines in the description to 79 characters in width.
> > > +Please limit lines in the description to 75 characters in
> > > width.  That
> > > +allows the description to format properly even when indented
> > > (e.g.
> > > by
> > > +"git log" or in email quotations).
> > 
> > Another nit (such is the way with anything coding standards'y):
> > isn't
> > 72 characters the usual recommendation? [1] Less importantly, we
> > don't
> > need to be so needlessly polite - "Limit lines..." is fine.
> 
> The default limit for "git citool" is 75.  Linus recommends 74 in the
> subsurface tree.  I think 75 is a reasonable limit since "git log"
> indents by 4 spaces and so 75-character lines won't wrap on an 80-
> column
> terminal.
> 
> I don't think being polite costs us anything here.

Sounds fair on both counts.

> Here's an incremental.
> 
> --8<--cut here-->8--
> 
> diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> index 721371e..2b262ca 100644
> --- a/CONTRIBUTING.rst
> +++ b/CONTRIBUTING.rst
> @@ -97,8 +97,8 @@ Where:
>  
>    briefly describes the change.  Use the the imperative form,
>    e.g. "Force SNAT for multiple gateway routers." or "Fix daemon
> exit
> -  for bad datapaths or flows."  This turns the summary into a
> -  sentence, so please end it with a period.
> +  for bad datapaths or flows."  Try to keep the summary short, about
> +  50 characters wide.
>  
>  The subject, minus the ``[PATCH /]`` prefix, becomes the first
> line of
>  the commit's change log message.

Looks good :)

Acked-by: Stephen Finucane 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] CONTRIBUTING.rst: Update patch summary and description style guidelines.

[Ben Pfaff]

On Thu, Dec 01, 2016 at 01:16:14PM +, Stephen Finucane wrote:
> On Wed, 2016-11-30 at 12:40 -0800, Ben Pfaff wrote:
> > Suggested-by: Joe Stringer 
> > Suggested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2016-Nov
> > ember/325513.html
> > Signed-off-by: Ben Pfaff 
> > ---
> >  CONTRIBUTING.rst | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> > index 867562e..721371e 100644
> > --- a/CONTRIBUTING.rst
> > +++ b/CONTRIBUTING.rst
> > @@ -94,7 +94,11 @@ Where:
> >    multiple distinct pieces of code.
> >  
> >  :
> > -  briefly describes the change.
> > +
> > +  briefly describes the change.  Use the the imperative form,
> > +  e.g. "Force SNAT for multiple gateway routers." or "Fix daemon
> > exit
> > +  for bad datapaths or flows."  This turns the summary into a
> > +  sentence, so please end it with a period.
> 
> This is super nitty (sorry), but the full stop takes up 1/50th of the
> available summary line characters [1] without adding any value :) I'd
> argue that its use certainly should be, at best, optional. If we're
> adding anything extra, I'd recommend that summary lines should be
> constrained to 50/51 characters where possible (vim does this itself).

Perhaps the period is more of a personal preference.  I'll drop it.

I find 50 characters to be almost impossible in many cases, myself, but
we can at least recommend it as something to try for.

> >  The subject, minus the ``[PATCH /]`` prefix, becomes the first
> > line of
> >  the commit's change log message.
> > @@ -106,7 +110,9 @@ The body of the email should start with a more
> > thorough description of the
> >  change.  This becomes the body of the commit message, following the
> > subject.
> >  There is no need to duplicate the summary given in the subject.
> >  
> > -Please limit lines in the description to 79 characters in width.
> > +Please limit lines in the description to 75 characters in
> > width.  That
> > +allows the description to format properly even when indented (e.g.
> > by
> > +"git log" or in email quotations).
> 
> Another nit (such is the way with anything coding standards'y): isn't
> 72 characters the usual recommendation? [1] Less importantly, we don't
> need to be so needlessly polite - "Limit lines..." is fine.

The default limit for "git citool" is 75.  Linus recommends 74 in the
subsurface tree.  I think 75 is a reasonable limit since "git log"
indents by 4 spaces and so 75-character lines won't wrap on an 80-column
terminal.

I don't think being polite costs us anything here.

Here's an incremental.

--8<--cut here-->8--

diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index 721371e..2b262ca 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -97,8 +97,8 @@ Where:
 
   briefly describes the change.  Use the the imperative form,
   e.g. "Force SNAT for multiple gateway routers." or "Fix daemon exit
-  for bad datapaths or flows."  This turns the summary into a
-  sentence, so please end it with a period.
+  for bad datapaths or flows."  Try to keep the summary short, about
+  50 characters wide.
 
 The subject, minus the ``[PATCH /]`` prefix, becomes the first line of
 the commit's change log message.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] CONTRIBUTING.rst: Update patch summary and description style guidelines.

[Stephen Finucane]

On Wed, 2016-11-30 at 12:40 -0800, Ben Pfaff wrote:
> Suggested-by: Joe Stringer 
> Suggested-at: https://mail.openvswitch.org/pipermail/ovs-dev/2016-Nov
> ember/325513.html
> Signed-off-by: Ben Pfaff 
> ---
>  CONTRIBUTING.rst | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> index 867562e..721371e 100644
> --- a/CONTRIBUTING.rst
> +++ b/CONTRIBUTING.rst
> @@ -94,7 +94,11 @@ Where:
>    multiple distinct pieces of code.
>  
>  :
> -  briefly describes the change.
> +
> +  briefly describes the change.  Use the the imperative form,
> +  e.g. "Force SNAT for multiple gateway routers." or "Fix daemon
> exit
> +  for bad datapaths or flows."  This turns the summary into a
> +  sentence, so please end it with a period.

This is super nitty (sorry), but the full stop takes up 1/50th of the
available summary line characters [1] without adding any value :) I'd
argue that its use certainly should be, at best, optional. If we're
adding anything extra, I'd recommend that summary lines should be
constrained to 50/51 characters where possible (vim does this itself).

>  The subject, minus the ``[PATCH /]`` prefix, becomes the first
> line of
>  the commit's change log message.
> @@ -106,7 +110,9 @@ The body of the email should start with a more
> thorough description of the
>  change.  This becomes the body of the commit message, following the
> subject.
>  There is no need to duplicate the summary given in the subject.
>  
> -Please limit lines in the description to 79 characters in width.
> +Please limit lines in the description to 75 characters in
> width.  That
> +allows the description to format properly even when indented (e.g.
> by
> +"git log" or in email quotations).

Another nit (such is the way with anything coding standards'y): isn't
72 characters the usual recommendation? [1] Less importantly, we don't
need to be so needlessly polite - "Limit lines..." is fine.

Stephen

[1] https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f
61f09c#.1maspcaci
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev