Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Doug Hellmann]

Excerpts from Zane Bitter's message of 2018-06-18 12:58:07 -0400:
> Replying to myself one more time...
> 
> On 12/06/18 17:35, Zane Bitter wrote:
> > On 11/06/18 18:49, Zane Bitter wrote:
> >> It's had a week to percolate (and I've seen quite a few people viewing 
> >> the etherpad), so here is the review:
> >>
> >> https://review.openstack.org/574479
> > 
> > In response to comments, I moved the change to the Project Team Guide
> > instead of the Contributor Guide (since the latter is aimed only at new 
> > contributors, but this is for everyone). The new review is here:
> > 
> > https://review.openstack.org/574888
> > 
> > The first review is still up, but it's now just adding links from the 
> > Contributor Guide to this new doc.
> 
> This is now live:
> 
> https://docs.openstack.org/project-team-guide/review-the-openstack-way.html
> 
> Thanks to everyone who contributed/reviewed/commented. Let's also 
> remember to make this a living document, so we all keep learning from 
> each other :)
> 
> cheers,
> Zane.
> 

+1

Nice work on this initiative, Julia and Zane.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

Replying to myself one more time...

On 12/06/18 17:35, Zane Bitter wrote:

On 11/06/18 18:49, Zane Bitter wrote:
It's had a week to percolate (and I've seen quite a few people viewing 
the etherpad), so here is the review:


https://review.openstack.org/574479


In response to comments, I moved the change to the Project Team Guide
instead of the Contributor Guide (since the latter is aimed only at new 
contributors, but this is for everyone). The new review is here:


https://review.openstack.org/574888

The first review is still up, but it's now just adding links from the 
Contributor Guide to this new doc.


This is now live:

https://docs.openstack.org/project-team-guide/review-the-openstack-way.html

Thanks to everyone who contributed/reviewed/commented. Let's also 
remember to make this a living document, so we all keep learning from 
each other :)


cheers,
Zane.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 11/06/18 18:49, Zane Bitter wrote:
It's had a week to percolate (and I've seen quite a few people viewing 
the etherpad), so here is the review:


https://review.openstack.org/574479


In response to comments, I moved the change to the Project Team Guide
instead of the Contributor Guide (since the latter is aimed only at new 
contributors, but this is for everyone). The new review is here:


https://review.openstack.org/574888

The first review is still up, but it's now just adding links from the 
Contributor Guide to this new doc.


cheers,
Zane.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Kendall Nelson]

Thanks for the patch Zane :)

-Kendall (diablo_rojo)

On Mon, Jun 11, 2018 at 3:50 PM Zane Bitter  wrote:

> On 04/06/18 10:13, Zane Bitter wrote:
> > On 31/05/18 14:35, Julia Kreger wrote:
> >> Back to the topic of nitpicking!
> >>
> >> I virtually sat down with Doug today and we hammered out the positive
> >> aspects that we feel like are the things that we as a community want
> >> to see as part of reviews coming out of this effort. The principles
> >> change[1] in governance has been updated as a result.
> >>
> >> I think we are at a point where we have to state high level
> >> principles, and then also update guidelines or other context providing
> >> documentation to re-enforce some of items covered in this
> >> discussion... not just to educate new contributors, but to serve as a
> >> checkpoint for existing reviewers when making the decision as to how
> >> to vote change set. The question then becomes where would such
> >> guidelines or documentation best fit?
> >
> > I think the contributor guide is the logical place for it. Kendall
> > pointed out this existing section:
> >
> >
> https://docs.openstack.org/contributors/code-and-documentation/using-gerrit.html#reviewing-changes
> >
> >
> > It could go in there, or perhaps we separate out the parts about when to
> > use which review scores into a separate page from the mechanics of how
> > to use Gerrit.
> >
> >> Should we explicitly detail the
> >> cause/effect that occurs? Should we convey contributor perceptions, or
> >> maybe even just link to this thread as there has been a massive amount
> >> of feedback raising valid cases, points, and frustrations.
> >>
> >> Personally, I'd lean towards a blended approach, but the question of
> >> where is one I'm unsure of. Thoughts?
> >
> > Let's crowdsource a set of heuristics that reviewers and contributors
> > should keep in mind when they're reviewing or having their changes
> > reviewed. I made a start on collecting ideas from this and past threads,
> > as well as my own reviewing experience, into a document that I've
> > presumptuously titled "How to Review Changes the OpenStack Way" (but
> > might be more accurately called "The Frank Sinatra Guide to Code Review"
> > at the moment):
> >
> > https://etherpad.openstack.org/p/review-the-openstack-way
> >
> > It's in an etherpad to make it easier for everyone to add their
> > suggestions and comments (folks in #openstack-tc have made some tweaks
> > already). After a suitable interval has passed to collect feedback, I'll
> > turn this into a contributor guide change.
>
> It's had a week to percolate (and I've seen quite a few people viewing
> the etherpad), so here is the review:
>
> https://review.openstack.org/574479
>
> - ZB
>
> > Have at it!
> >
> > cheers,
> > Zane.
> >
> >> -Julia
> >>
> >> [1]: https://review.openstack.org/#/c/570940/
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 04/06/18 10:13, Zane Bitter wrote:

On 31/05/18 14:35, Julia Kreger wrote:

Back to the topic of nitpicking!

I virtually sat down with Doug today and we hammered out the positive
aspects that we feel like are the things that we as a community want
to see as part of reviews coming out of this effort. The principles
change[1] in governance has been updated as a result.

I think we are at a point where we have to state high level
principles, and then also update guidelines or other context providing
documentation to re-enforce some of items covered in this
discussion... not just to educate new contributors, but to serve as a
checkpoint for existing reviewers when making the decision as to how
to vote change set. The question then becomes where would such
guidelines or documentation best fit?


I think the contributor guide is the logical place for it. Kendall 
pointed out this existing section:


https://docs.openstack.org/contributors/code-and-documentation/using-gerrit.html#reviewing-changes 



It could go in there, or perhaps we separate out the parts about when to 
use which review scores into a separate page from the mechanics of how 
to use Gerrit.



Should we explicitly detail the
cause/effect that occurs? Should we convey contributor perceptions, or
maybe even just link to this thread as there has been a massive amount
of feedback raising valid cases, points, and frustrations.

Personally, I'd lean towards a blended approach, but the question of
where is one I'm unsure of. Thoughts?


Let's crowdsource a set of heuristics that reviewers and contributors 
should keep in mind when they're reviewing or having their changes 
reviewed. I made a start on collecting ideas from this and past threads, 
as well as my own reviewing experience, into a document that I've 
presumptuously titled "How to Review Changes the OpenStack Way" (but 
might be more accurately called "The Frank Sinatra Guide to Code Review" 
at the moment):


https://etherpad.openstack.org/p/review-the-openstack-way

It's in an etherpad to make it easier for everyone to add their 
suggestions and comments (folks in #openstack-tc have made some tweaks 
already). After a suitable interval has passed to collect feedback, I'll 
turn this into a contributor guide change.


It's had a week to percolate (and I've seen quite a few people viewing 
the etherpad), so here is the review:


https://review.openstack.org/574479

- ZB


Have at it!

cheers,
Zane.


-Julia

[1]: https://review.openstack.org/#/c/570940/



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Assaf Muller]

On Tue, May 29, 2018 at 12:41 PM, Mathieu Gagné  wrote:
> Hi Julia,
>
> Thanks for the follow up on this topic.
>
> On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
>  wrote:
>>
>> These things are not just frustrating, but also very inhibiting for
>> part time contributors such as students who may also be time limited.
>> Or an operator who noticed something that was clearly a bug and that
>> put forth a very minor fix and doesn't have the time to revise it over
>> and over.
>>
>
> What I found frustrating is receiving *only* nitpicks, addressing them
> to only receive more nitpicks (sometimes from the same reviewer) with
> no substantial review on the change itself afterward.
> I wouldn't mind addressing nitpicks if more substantial reviews were
> made in a timely fashion.

The behavior that I've tried to promote in communities I've partaken
is: If your review is comprised solely of nits, either abandon it, or
don't -1.

>
> --
> Mathieu
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Rochelle Grober]

Zane Bitter wrote:
> On 31/05/18 14:35, Julia Kreger wrote:
> > Back to the topic of nitpicking!
> >
> > I virtually sat down with Doug today and we hammered out the positive
> > aspects that we feel like are the things that we as a community want
> > to see as part of reviews coming out of this effort. The principles
> > change[1] in governance has been updated as a result.
> >
> > I think we are at a point where we have to state high level
> > principles, and then also update guidelines or other context providing
> > documentation to re-enforce some of items covered in this
> > discussion... not just to educate new contributors, but to serve as a
> > checkpoint for existing reviewers when making the decision as to how
> > to vote change set. The question then becomes where would such
> > guidelines or documentation best fit?
> 
> I think the contributor guide is the logical place for it. Kendall pointed 
> out this
> existing section:
> 
> https://docs.openstack.org/contributors/code-and-documentation/using-
> gerrit.html#reviewing-changes
> 
> It could go in there, or perhaps we separate out the parts about when to use
> which review scores into a separate page from the mechanics of how to use
> Gerrit.
> 
> > Should we explicitly detail the
> > cause/effect that occurs? Should we convey contributor perceptions, or
> > maybe even just link to this thread as there has been a massive amount
> > of feedback raising valid cases, points, and frustrations.
> >
> > Personally, I'd lean towards a blended approach, but the question of
> > where is one I'm unsure of. Thoughts?
> 
> Let's crowdsource a set of heuristics that reviewers and contributors should
> keep in mind when they're reviewing or having their changes reviewed. I
> made a start on collecting ideas from this and past threads, as well as my own
> reviewing experience, into a document that I've presumptuously titled "How
> to Review Changes the OpenStack Way" (but might be more accurately called
> "The Frank Sinatra Guide to Code Review"
> at the moment):
> 
> https://etherpad.openstack.org/p/review-the-openstack-way
> 
> It's in an etherpad to make it easier for everyone to add their suggestions
> and comments (folks in #openstack-tc have made some tweaks already).
> After a suitable interval has passed to collect feedback, I'll turn this into 
> a
> contributor guide change.

I offer the suggestion that there are some real examples of Good/Not Good in 
the document or maybe an addendum.  Since we have many non-native speakers in 
our community, examples are like pictures -- worth a thousand foreign words;-)

Maybe Zhipeng has a few favorites to supply.  I would suggest both score and 
comment to go with score.  In some cases, the example would show how to score 
and avoid nitpicking, in others, valid scores, but comments that are reasonable 
or not for the score.

--Rocky
 
> Have at it!
> 
> cheers,
> Zane.
> 
> > -Julia
> >
> > [1]: https://review.openstack.org/#/c/570940/
> 
> __
> 
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-
> requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jeremy Stanley]

On 2018-06-04 14:28:28 -0700 (-0700), Amy Marrich wrote:
> On Mon, Jun 4, 2018 at 7:29 AM, Zane Bitter  wrote:
> > On 04/06/18 10:19, Amy Marrich wrote:
> > > [...]
> > > I'll read in more detail, but do we want to add rollcall-vote?
> > 
> > Is it used anywhere other than in the governance repo? We certainly could
> > add it, but it didn't seem like a top priority.
> 
> Not sure it is to be honest.:)

The infra-specs repo uses it to solicit Infra Council votes; the
governance-uc, openstack-specs and transparency-policy repos also
use it for similar reasons to the governance repo. But no, it's not
common enough I'd bother to mention it in any sort of documentation
aimed at the general reviewer base.
-- 
Jeremy Stanley


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Amy Marrich]

Zane,

Not sure it is to be honest.:)

Amy (spotz)

On Mon, Jun 4, 2018 at 7:29 AM, Zane Bitter  wrote:

> On 04/06/18 10:19, Amy Marrich wrote:
>
>> Zane,
>>
>> I'll read in more detail, but do we want to add rollcall-vote?
>>
>
> Is it used anywhere other than in the governance repo? We certainly could
> add it, but it didn't seem like a top priority.
>
> - ZB
>
> Amy (spotz)
>>
>>
>> On Mon, Jun 4, 2018 at 7:13 AM, Zane Bitter > zbit...@redhat.com>> wrote:
>>
>> On 31/05/18 14:35, Julia Kreger wrote:
>>
>> Back to the topic of nitpicking!
>>
>> I virtually sat down with Doug today and we hammered out the
>> positive
>> aspects that we feel like are the things that we as a community
>> want
>> to see as part of reviews coming out of this effort. The
>> principles
>> change[1] in governance has been updated as a result.
>>
>> I think we are at a point where we have to state high level
>> principles, and then also update guidelines or other context
>> providing
>> documentation to re-enforce some of items covered in this
>> discussion... not just to educate new contributors, but to serve
>> as a
>> checkpoint for existing reviewers when making the decision as to
>> how
>> to vote change set. The question then becomes where would such
>> guidelines or documentation best fit?
>>
>>
>> I think the contributor guide is the logical place for it. Kendall
>> pointed out this existing section:
>>
>> https://docs.openstack.org/contributors/code-and-documentati
>> on/using-gerrit.html#reviewing-changes
>> > ion/using-gerrit.html#reviewing-changes>
>>
>> It could go in there, or perhaps we separate out the parts about
>> when to use which review scores into a separate page from the
>> mechanics of how to use Gerrit.
>>
>> Should we explicitly detail the
>> cause/effect that occurs? Should we convey contributor
>> perceptions, or
>> maybe even just link to this thread as there has been a massive
>> amount
>> of feedback raising valid cases, points, and frustrations.
>>
>> Personally, I'd lean towards a blended approach, but the question
>> of
>> where is one I'm unsure of. Thoughts?
>>
>>
>> Let's crowdsource a set of heuristics that reviewers and
>> contributors should keep in mind when they're reviewing or having
>> their changes reviewed. I made a start on collecting ideas from this
>> and past threads, as well as my own reviewing experience, into a
>> document that I've presumptuously titled "How to Review Changes the
>> OpenStack Way" (but might be more accurately called "The Frank
>> Sinatra Guide to Code Review" at the moment):
>>
>> https://etherpad.openstack.org/p/review-the-openstack-way
>> 
>>
>> It's in an etherpad to make it easier for everyone to add their
>> suggestions and comments (folks in #openstack-tc have made some
>> tweaks already). After a suitable interval has passed to collect
>> feedback, I'll turn this into a contributor guide change.
>>
>> Have at it!
>>
>> cheers,
>> Zane.
>>
>>
>> -Julia
>>
>> [1]: https://review.openstack.org/#/c/570940/
>> 
>>
>>
>> 
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> > >
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> 
>>
>>
>>
>>
>> 
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscrib
>> e
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 04/06/18 10:19, Amy Marrich wrote:

Zane,

I'll read in more detail, but do we want to add rollcall-vote?


Is it used anywhere other than in the governance repo? We certainly 
could add it, but it didn't seem like a top priority.


- ZB


Amy (spotz)

On Mon, Jun 4, 2018 at 7:13 AM, Zane Bitter > wrote:


On 31/05/18 14:35, Julia Kreger wrote:

Back to the topic of nitpicking!

I virtually sat down with Doug today and we hammered out the
positive
aspects that we feel like are the things that we as a community want
to see as part of reviews coming out of this effort. The principles
change[1] in governance has been updated as a result.

I think we are at a point where we have to state high level
principles, and then also update guidelines or other context
providing
documentation to re-enforce some of items covered in this
discussion... not just to educate new contributors, but to serve
as a
checkpoint for existing reviewers when making the decision as to how
to vote change set. The question then becomes where would such
guidelines or documentation best fit?


I think the contributor guide is the logical place for it. Kendall
pointed out this existing section:


https://docs.openstack.org/contributors/code-and-documentation/using-gerrit.html#reviewing-changes



It could go in there, or perhaps we separate out the parts about
when to use which review scores into a separate page from the
mechanics of how to use Gerrit.

Should we explicitly detail the
cause/effect that occurs? Should we convey contributor
perceptions, or
maybe even just link to this thread as there has been a massive
amount
of feedback raising valid cases, points, and frustrations.

Personally, I'd lean towards a blended approach, but the question of
where is one I'm unsure of. Thoughts?


Let's crowdsource a set of heuristics that reviewers and
contributors should keep in mind when they're reviewing or having
their changes reviewed. I made a start on collecting ideas from this
and past threads, as well as my own reviewing experience, into a
document that I've presumptuously titled "How to Review Changes the
OpenStack Way" (but might be more accurately called "The Frank
Sinatra Guide to Code Review" at the moment):

https://etherpad.openstack.org/p/review-the-openstack-way


It's in an etherpad to make it easier for everyone to add their
suggestions and comments (folks in #openstack-tc have made some
tweaks already). After a suitable interval has passed to collect
feedback, I'll turn this into a contributor guide change.

Have at it!

cheers,
Zane.


-Julia

[1]: https://review.openstack.org/#/c/570940/



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe:
openstack-dev-requ...@lists.openstack.org?subject:unsubscribe

http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev





__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Amy Marrich]

Zane,

I'll read in more detail, but do we want to add rollcall-vote?

Amy (spotz)

On Mon, Jun 4, 2018 at 7:13 AM, Zane Bitter  wrote:

> On 31/05/18 14:35, Julia Kreger wrote:
>
>> Back to the topic of nitpicking!
>>
>> I virtually sat down with Doug today and we hammered out the positive
>> aspects that we feel like are the things that we as a community want
>> to see as part of reviews coming out of this effort. The principles
>> change[1] in governance has been updated as a result.
>>
>> I think we are at a point where we have to state high level
>> principles, and then also update guidelines or other context providing
>> documentation to re-enforce some of items covered in this
>> discussion... not just to educate new contributors, but to serve as a
>> checkpoint for existing reviewers when making the decision as to how
>> to vote change set. The question then becomes where would such
>> guidelines or documentation best fit?
>>
>
> I think the contributor guide is the logical place for it. Kendall pointed
> out this existing section:
>
> https://docs.openstack.org/contributors/code-and-documentati
> on/using-gerrit.html#reviewing-changes
>
> It could go in there, or perhaps we separate out the parts about when to
> use which review scores into a separate page from the mechanics of how to
> use Gerrit.
>
> Should we explicitly detail the
>> cause/effect that occurs? Should we convey contributor perceptions, or
>> maybe even just link to this thread as there has been a massive amount
>> of feedback raising valid cases, points, and frustrations.
>>
>> Personally, I'd lean towards a blended approach, but the question of
>> where is one I'm unsure of. Thoughts?
>>
>
> Let's crowdsource a set of heuristics that reviewers and contributors
> should keep in mind when they're reviewing or having their changes
> reviewed. I made a start on collecting ideas from this and past threads, as
> well as my own reviewing experience, into a document that I've
> presumptuously titled "How to Review Changes the OpenStack Way" (but might
> be more accurately called "The Frank Sinatra Guide to Code Review" at the
> moment):
>
> https://etherpad.openstack.org/p/review-the-openstack-way
>
> It's in an etherpad to make it easier for everyone to add their
> suggestions and comments (folks in #openstack-tc have made some tweaks
> already). After a suitable interval has passed to collect feedback, I'll
> turn this into a contributor guide change.
>
> Have at it!
>
> cheers,
> Zane.
>
>
> -Julia
>>
>> [1]: https://review.openstack.org/#/c/570940/
>>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 31/05/18 14:35, Julia Kreger wrote:

Back to the topic of nitpicking!

I virtually sat down with Doug today and we hammered out the positive
aspects that we feel like are the things that we as a community want
to see as part of reviews coming out of this effort. The principles
change[1] in governance has been updated as a result.

I think we are at a point where we have to state high level
principles, and then also update guidelines or other context providing
documentation to re-enforce some of items covered in this
discussion... not just to educate new contributors, but to serve as a
checkpoint for existing reviewers when making the decision as to how
to vote change set. The question then becomes where would such
guidelines or documentation best fit?


I think the contributor guide is the logical place for it. Kendall 
pointed out this existing section:


https://docs.openstack.org/contributors/code-and-documentation/using-gerrit.html#reviewing-changes

It could go in there, or perhaps we separate out the parts about when to 
use which review scores into a separate page from the mechanics of how 
to use Gerrit.



Should we explicitly detail the
cause/effect that occurs? Should we convey contributor perceptions, or
maybe even just link to this thread as there has been a massive amount
of feedback raising valid cases, points, and frustrations.

Personally, I'd lean towards a blended approach, but the question of
where is one I'm unsure of. Thoughts?


Let's crowdsource a set of heuristics that reviewers and contributors 
should keep in mind when they're reviewing or having their changes 
reviewed. I made a start on collecting ideas from this and past threads, 
as well as my own reviewing experience, into a document that I've 
presumptuously titled "How to Review Changes the OpenStack Way" (but 
might be more accurately called "The Frank Sinatra Guide to Code Review" 
at the moment):


https://etherpad.openstack.org/p/review-the-openstack-way

It's in an etherpad to make it easier for everyone to add their 
suggestions and comments (folks in #openstack-tc have made some tweaks 
already). After a suitable interval has passed to collect feedback, I'll 
turn this into a contributor guide change.


Have at it!

cheers,
Zane.


-Julia

[1]: https://review.openstack.org/#/c/570940/


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zhipeng Huang]

For me nitpicking during review is really not a good experience, however i
do think we should tolerate at least one round of nitpicking.

On another aspect, the nitpicking review culture also in some way
encourage, and provide legitimacy in some way, to the padding activities.
People are feeling ok about "fixing dictionary" as we joked.



On Fri, Jun 1, 2018 at 4:55 AM, Jeremy Stanley  wrote:

> On 2018-05-31 16:49:13 -0400 (-0400), John Dennis wrote:
> > On 05/30/2018 08:23 PM, Jeremy Stanley wrote:
> > > I think this is orthogonal to the thread. The idea is that we should
> > > avoid nettling contributors over minor imperfections in their
> > > submissions (grammatical, spelling or typographical errors in code
> > > comments and documentation, mild inefficiencies in implementations,
> > > et cetera). Clearly we shouldn't merge broken features, changes
> > > which fail tests/linters, and so on. For me the rule of thumb is,
> > > "will the software be better or worse if this is merged?" It's not
> > > about perfection or imperfection, it's about incremental
> > > improvement. If a proposed change is an improvement, that's enough.
> > > If it's not perfect... well, that's just opportunity for more
> > > improvement later.
> >
> > I appreciate the sentiment concerning accepting any improvement yet on
> the
> > other hand waiting for improvements to the patch to occur later is
> folly, it
> > won't happen.
> >
> > Those of us familiar with working with large bodies of code from multiple
> > authors spanning an extended time period will tell you it's very
> confusing
> > when it's obvious most of the code follows certain conventions but there
> are
> > odd exceptions (often without comments). This inevitably leads to
> investing
> > a lot of time trying to understand why the exception exists because
> "clearly
> > it's there for a reason and I'm just missing the rationale" At that point
> > the reason for the inconsistency is lost.
> >
> > At the end of the day it is more important to keep the code base clean
> and
> > consistent for those that follow than it is to coddle in the near term.
>
> Sure, I suppose it comes down to your definition of "improvement." I
> don't consider a change proposing incomplete or unmaintainable code
> to be an improvement. On the other hand I think it's fine to approve
> changes which are "good enough" even if there's room for
> improvement, so long as they're "good enough" that you're fine with
> them possibly never being improved on due to shifts in priorities.
> I'm certainly not suggesting that it's a good idea to merge
> technical debt with the expectation that someone will find time to
> solve it later (any more than it's okay to merge obvious bugs in
> hopes someone will come along and fix them for you).
> --
> Jeremy Stanley
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>


-- 
Zhipeng (Howard) Huang

Standard Engineer
IT Standard & Patent/IT Product Line
Huawei Technologies Co,. Ltd
Email: huangzhip...@huawei.com
Office: Huawei Industrial Base, Longgang, Shenzhen

(Previous)
Research Assistant
Mobile Ad-Hoc Network Lab, Calit2
University of California, Irvine
Email: zhipe...@uci.edu
Office: Calit2 Building Room 2402

OpenStack, OPNFV, OpenDaylight, OpenCompute Aficionado
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jeremy Stanley]

On 2018-05-31 16:49:13 -0400 (-0400), John Dennis wrote:
> On 05/30/2018 08:23 PM, Jeremy Stanley wrote:
> > I think this is orthogonal to the thread. The idea is that we should
> > avoid nettling contributors over minor imperfections in their
> > submissions (grammatical, spelling or typographical errors in code
> > comments and documentation, mild inefficiencies in implementations,
> > et cetera). Clearly we shouldn't merge broken features, changes
> > which fail tests/linters, and so on. For me the rule of thumb is,
> > "will the software be better or worse if this is merged?" It's not
> > about perfection or imperfection, it's about incremental
> > improvement. If a proposed change is an improvement, that's enough.
> > If it's not perfect... well, that's just opportunity for more
> > improvement later.
> 
> I appreciate the sentiment concerning accepting any improvement yet on the
> other hand waiting for improvements to the patch to occur later is folly, it
> won't happen.
> 
> Those of us familiar with working with large bodies of code from multiple
> authors spanning an extended time period will tell you it's very confusing
> when it's obvious most of the code follows certain conventions but there are
> odd exceptions (often without comments). This inevitably leads to investing
> a lot of time trying to understand why the exception exists because "clearly
> it's there for a reason and I'm just missing the rationale" At that point
> the reason for the inconsistency is lost.
> 
> At the end of the day it is more important to keep the code base clean and
> consistent for those that follow than it is to coddle in the near term.

Sure, I suppose it comes down to your definition of "improvement." I
don't consider a change proposing incomplete or unmaintainable code
to be an improvement. On the other hand I think it's fine to approve
changes which are "good enough" even if there's room for
improvement, so long as they're "good enough" that you're fine with
them possibly never being improved on due to shifts in priorities.
I'm certainly not suggesting that it's a good idea to merge
technical debt with the expectation that someone will find time to
solve it later (any more than it's okay to merge obvious bugs in
hopes someone will come along and fix them for you).
-- 
Jeremy Stanley


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[John Dennis]

On 05/30/2018 08:23 PM, Jeremy Stanley wrote:

I think this is orthogonal to the thread. The idea is that we should
avoid nettling contributors over minor imperfections in their
submissions (grammatical, spelling or typographical errors in code
comments and documentation, mild inefficiencies in implementations,
et cetera). Clearly we shouldn't merge broken features, changes
which fail tests/linters, and so on. For me the rule of thumb is,
"will the software be better or worse if this is merged?" It's not
about perfection or imperfection, it's about incremental
improvement. If a proposed change is an improvement, that's enough.
If it's not perfect... well, that's just opportunity for more
improvement later.


I appreciate the sentiment concerning accepting any improvement yet on 
the other hand waiting for improvements to the patch to occur later is 
folly, it won't happen.


Those of us familiar with working with large bodies of code from 
multiple authors spanning an extended time period will tell you it's 
very confusing when it's obvious most of the code follows certain 
conventions but there are odd exceptions (often without comments). This 
inevitably leads to investing a lot of time trying to understand why the 
exception exists because "clearly it's there for a reason and I'm just 
missing the rationale" At that point the reason for the inconsistency is 
lost.


At the end of the day it is more important to keep the code base clean 
and consistent for those that follow than it is to coddle in the near term.


--
John Dennis

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Julia Kreger]

Back to the topic of nitpicking!

I virtually sat down with Doug today and we hammered out the positive
aspects that we feel like are the things that we as a community want
to see as part of reviews coming out of this effort. The principles
change[1] in governance has been updated as a result.

I think we are at a point where we have to state high level
principles, and then also update guidelines or other context providing
documentation to re-enforce some of items covered in this
discussion... not just to educate new contributors, but to serve as a
checkpoint for existing reviewers when making the decision as to how
to vote change set. The question then becomes where would such
guidelines or documentation best fit? Should we explicitly detail the
cause/effect that occurs? Should we convey contributor perceptions, or
maybe even just link to this thread as there has been a massive amount
of feedback raising valid cases, points, and frustrations.

Personally, I'd lean towards a blended approach, but the question of
where is one I'm unsure of. Thoughts?

-Julia

[1]: https://review.openstack.org/#/c/570940/

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Thierry Carrez]

Davanum Srinivas wrote:

"master should be always deployable and fully backward compatible and
so we cant let anything in anytime that could possibly regress anyone"

Should we change that attitude too? Anyone agree? disagree?

Thanks,
Dims


I'll definitely jump at this one.

I've always thought (and shared on the ML several times now) that our
implied
but not explicit support for CD from any random commit was a bad thing.

While I think it's good to support the idea that master is always
deployable, I
do not think it is a good mindset to think that every commit is a
"release" and
therefore should be supported until the end of time. We have a coordinated
release for a reason, and I think design decisions and fixes should be
based on
the assumption that a release is a release and the point at which we
need to be
cognizant and caring about keeping backward compatibility. Doing that for
every single commit is not ideal for the overall health of the product, IMO.



It's more than just a CD guarantee, while from a quick glance it would seem like
that's the only value it goes much deeper than that. Ensuring that every commit
works, is deployable, and maintains backwards compatibility is what enables us
to have such a high quality end result at release time. Quite frankly it's
looking at every commit as always being a working unit that enables us to manage
a project this size at this velocity. Even if people assume no one is actually
CDing the projects(which we shouldn't), it's a flawed assumption to think that
everyone is running strictly the same code as what's in the release tarballs. I
can't think of any production cloud out there that doesn't carry patches to fix
things encountered in the real world. Or look at stable maint we regularly need
to backport fixes to fix bugs found after release. If we can't rely on these to
always work this makes our life much more difficult, both as upstream
maintainers but also as downstream consumers of OpenStack.

The other aspect to look at here is just the review mindset, supporting every
every commit is useable puts reviewers in the mindset to consider things like
backwards compatibility and deployability when looking at proposed changes. If
we stop looking for these potential issues, we t will also cause many more bugs
to be in our released code. To simply discount this as only a release concern
and punt this kind of scrutiny until it's time to release is not only going to
make release time much more stressful. Also, our testing is built to try and
ensure every commit works **before** we merge it. If we decided to take this
stance as a community then we should really just rip out all the testing,
because that's what it's there to verify and help us make sure we don't land a
change that doesn't work. If we don't actually care about that making sure every
commit is deployable we are wasting quite a lot of resources on it.


"rip out all testing" is probably taking it too far Matt.

  Instead of perfection when merging, we should look for iteration and
reverts. That's what i would like to see. I am not asking for a
"Commit-Then-Review" like the ASF. I want us to be just be practical
and have some leeway to iterate / update / experiment instead of
absolute perfection from all angles. We should move the needle at
least a bit away from it.


Right... There might be a reasonable middle ground between "every commit 
on master must be backward-compatible" and "rip out all testing" that 
allows us to routinely revert broken feature commits (as long as they 
don't cross a release boundary).


To be fair, I'm pretty sure that's already the case: we did revert 
feature commits on master in the past, therefore breaking backward 
compatibility if someone started to use that feature right away. It's 
the issue with implicit rules: everyone interprets them the way they 
want... So I think that could use some explicit clarification.


[ This tangent should probably gets its own thread to not disrupt the 
no-nitpicking discussion ]


--
Thierry Carrez (ttx)

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Ed Leafe]

On May 30, 2018, at 5:11 AM, Dmitry Tantsur  wrote:

> Whatever decision the TC takes, I would like it to make sure that we don't 
> paint putting -1 as a bad act. Nor do I want "if you care, just follow-up" to 
> be an excuse for putting up bad contributions.
> 
> Additionally, I would like to have something saying that a -1 is valid and 
> appropriate, if a contribution substantially increases the project's 
> technical debt. After already spending *days* refactoring ironic unit tests, 
> I will -1 the hell out of a patch that will try to bring them back to their 
> initial state, I promise :)

Yes to this. -1 should never mean anything other than "some more work needs to 
be done before this can merge". It most certainly does not mean "your code is 
bad and you should feel terrible".

While this started as a discussion about reducing nitpicking, which I think we 
can all embrace, we shouldn't let it slide into imagining that contributors are 
such fragile things that pointing out an error/problem in the code is seen as a 
personal attack. Of course you should not be mean when you do so. But that's 
very, very rare in my OpenStack experience. Nitpicking, on the other hand, is 
much more prevalent, and I welcome these efforts to reduce it.

-- Ed Leafe







signature.asc
Description: Message signed with OpenPGP
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Davanum Srinivas]

On Wed, May 30, 2018 at 6:09 PM, Matthew Treinish  wrote:
> On Thu, May 31, 2018 at 12:21:35AM +, Fox, Kevin M wrote:
>> To play devils advocate and as someone that has had to git bisect an ugly 
>> regression once I still think its important not to break trunk. It can be 
>> much harder to deal with difficult issues like that if trunk frequently 
>> breaks.
>>
>> Thanks,
>> Kevin
>> 
>> From: Sean McGinnis [sean.mcgin...@gmx.com]
>> Sent: Wednesday, May 30, 2018 5:01 PM
>> To: openstack-dev@lists.openstack.org
>> Subject: Re: [openstack-dev] [tc][all] A culture change (nitpicking)
>>
>> > "master should be always deployable and fully backward compatible and
>> > so we cant let anything in anytime that could possibly regress anyone"
>> >
>> > Should we change that attitude too? Anyone agree? disagree?
>> >
>> > Thanks,
>> > Dims
>> >
>> I'll definitely jump at this one.
>>
>> I've always thought (and shared on the ML several times now) that our
>> implied
>> but not explicit support for CD from any random commit was a bad thing.
>>
>> While I think it's good to support the idea that master is always
>> deployable, I
>> do not think it is a good mindset to think that every commit is a
>> "release" and
>> therefore should be supported until the end of time. We have a coordinated
>> release for a reason, and I think design decisions and fixes should be
>> based on
>> the assumption that a release is a release and the point at which we
>> need to be
>> cognizant and caring about keeping backward compatibility. Doing that for
>> every single commit is not ideal for the overall health of the product, IMO.
>>
>
> It's more than just a CD guarantee, while from a quick glance it would seem 
> like
> that's the only value it goes much deeper than that. Ensuring that every 
> commit
> works, is deployable, and maintains backwards compatibility is what enables us
> to have such a high quality end result at release time. Quite frankly it's
> looking at every commit as always being a working unit that enables us to 
> manage
> a project this size at this velocity. Even if people assume no one is actually
> CDing the projects(which we shouldn't), it's a flawed assumption to think that
> everyone is running strictly the same code as what's in the release tarballs. 
> I
> can't think of any production cloud out there that doesn't carry patches to 
> fix
> things encountered in the real world. Or look at stable maint we regularly 
> need
> to backport fixes to fix bugs found after release. If we can't rely on these 
> to
> always work this makes our life much more difficult, both as upstream
> maintainers but also as downstream consumers of OpenStack.
>
> The other aspect to look at here is just the review mindset, supporting every
> every commit is useable puts reviewers in the mindset to consider things like
> backwards compatibility and deployability when looking at proposed changes. If
> we stop looking for these potential issues, we t will also cause many more 
> bugs
> to be in our released code. To simply discount this as only a release concern
> and punt this kind of scrutiny until it's time to release is not only going to
> make release time much more stressful. Also, our testing is built to try and
> ensure every commit works **before** we merge it. If we decided to take this
> stance as a community then we should really just rip out all the testing,
> because that's what it's there to verify and help us make sure we don't land a
> change that doesn't work. If we don't actually care about that making sure 
> every
> commit is deployable we are wasting quite a lot of resources on it.

"rip out all testing" is probably taking it too far Matt.

 Instead of perfection when merging, we should look for iteration and
reverts. That's what i would like to see. I am not asking for a
"Commit-Then-Review" like the ASF. I want us to be just be practical
and have some leeway to iterate / update / experiment instead of
absolute perfection from all angles. We should move the needle at
least a bit away from it.

>
> -Matt Treinish
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Davanum Srinivas :: https://twitter.com/dims

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Matthew Treinish]

On Thu, May 31, 2018 at 12:21:35AM +, Fox, Kevin M wrote:
> To play devils advocate and as someone that has had to git bisect an ugly 
> regression once I still think its important not to break trunk. It can be 
> much harder to deal with difficult issues like that if trunk frequently 
> breaks.
> 
> Thanks,
> Kevin
> 
> From: Sean McGinnis [sean.mcgin...@gmx.com]
> Sent: Wednesday, May 30, 2018 5:01 PM
> To: openstack-dev@lists.openstack.org
> Subject: Re: [openstack-dev] [tc][all] A culture change (nitpicking)
> 
> > "master should be always deployable and fully backward compatible and
> > so we cant let anything in anytime that could possibly regress anyone"
> >
> > Should we change that attitude too? Anyone agree? disagree?
> >
> > Thanks,
> > Dims
> >
> I'll definitely jump at this one.
> 
> I've always thought (and shared on the ML several times now) that our
> implied
> but not explicit support for CD from any random commit was a bad thing.
> 
> While I think it's good to support the idea that master is always
> deployable, I
> do not think it is a good mindset to think that every commit is a
> "release" and
> therefore should be supported until the end of time. We have a coordinated
> release for a reason, and I think design decisions and fixes should be
> based on
> the assumption that a release is a release and the point at which we
> need to be
> cognizant and caring about keeping backward compatibility. Doing that for
> every single commit is not ideal for the overall health of the product, IMO.
> 

It's more than just a CD guarantee, while from a quick glance it would seem like
that's the only value it goes much deeper than that. Ensuring that every commit
works, is deployable, and maintains backwards compatibility is what enables us
to have such a high quality end result at release time. Quite frankly it's
looking at every commit as always being a working unit that enables us to manage
a project this size at this velocity. Even if people assume no one is actually
CDing the projects(which we shouldn't), it's a flawed assumption to think that
everyone is running strictly the same code as what's in the release tarballs. I
can't think of any production cloud out there that doesn't carry patches to fix
things encountered in the real world. Or look at stable maint we regularly need
to backport fixes to fix bugs found after release. If we can't rely on these to
always work this makes our life much more difficult, both as upstream
maintainers but also as downstream consumers of OpenStack.

The other aspect to look at here is just the review mindset, supporting every
every commit is useable puts reviewers in the mindset to consider things like
backwards compatibility and deployability when looking at proposed changes. If
we stop looking for these potential issues, we t will also cause many more bugs
to be in our released code. To simply discount this as only a release concern
and punt this kind of scrutiny until it's time to release is not only going to
make release time much more stressful. Also, our testing is built to try and
ensure every commit works **before** we merge it. If we decided to take this
stance as a community then we should really just rip out all the testing,
because that's what it's there to verify and help us make sure we don't land a
change that doesn't work. If we don't actually care about that making sure every
commit is deployable we are wasting quite a lot of resources on it.

-Matt Treinish


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Davanum Srinivas]

On Wed, May 30, 2018 at 5:23 PM, Jeremy Stanley  wrote:
> On 2018-05-30 14:50:11 -0700 (-0700), Davanum Srinivas wrote:
> [...]
>> Let me poke at this a bit. Some of the projects do say (not in so
>> many words):
>>
>> "master should be always deployable and fully backward compatible and
>> so we cant let anything in anytime that could possibly regress anyone"
>>
>> Should we change that attitude too? Anyone agree? disagree?
>
> I think this is orthogonal to the thread. The idea is that we should
> avoid nettling contributors over minor imperfections in their
> submissions (grammatical, spelling or typographical errors in code
> comments and documentation, mild inefficiencies in implementations,
> et cetera). Clearly we shouldn't merge broken features, changes
> which fail tests/linters, and so on. For me the rule of thumb is,
> "will the software be better or worse if this is merged?" It's not
> about perfection or imperfection, it's about incremental
> improvement. If a proposed change is an improvement, that's enough.
> If it's not perfect... well, that's just opportunity for more
> improvement later.

Well said Jeremy!

> --
> Jeremy Stanley
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Davanum Srinivas :: https://twitter.com/dims

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jeremy Stanley]

On 2018-05-31 00:21:35 + (+), Fox, Kevin M wrote:
> To play devils advocate and as someone that has had to git bisect
> an ugly regression once I still think its important not to break
> trunk. It can be much harder to deal with difficult issues like
> that if trunk frequently breaks.
[...]

Agreed. We made a choice as a community early on to avoid doing
that. (Almost) always deployable is (almost) always testable; if
trunk is broken, then as a developer working on a bugfix or new
feature you have to hunt for a known-working state in the history to
develop against and then rebase onto all the broken and cross your
fingers that you're not making the job of whoever has to untangle
trunk before release that much harder.
-- 
Jeremy Stanley


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jeremy Stanley]

On 2018-05-30 14:50:11 -0700 (-0700), Davanum Srinivas wrote:
[...]
> Let me poke at this a bit. Some of the projects do say (not in so
> many words):
> 
> "master should be always deployable and fully backward compatible and
> so we cant let anything in anytime that could possibly regress anyone"
> 
> Should we change that attitude too? Anyone agree? disagree?

I think this is orthogonal to the thread. The idea is that we should
avoid nettling contributors over minor imperfections in their
submissions (grammatical, spelling or typographical errors in code
comments and documentation, mild inefficiencies in implementations,
et cetera). Clearly we shouldn't merge broken features, changes
which fail tests/linters, and so on. For me the rule of thumb is,
"will the software be better or worse if this is merged?" It's not
about perfection or imperfection, it's about incremental
improvement. If a proposed change is an improvement, that's enough.
If it's not perfect... well, that's just opportunity for more
improvement later.
-- 
Jeremy Stanley


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Fox, Kevin M]

To play devils advocate and as someone that has had to git bisect an ugly 
regression once I still think its important not to break trunk. It can be much 
harder to deal with difficult issues like that if trunk frequently breaks.

Thanks,
Kevin

From: Sean McGinnis [sean.mcgin...@gmx.com]
Sent: Wednesday, May 30, 2018 5:01 PM
To: openstack-dev@lists.openstack.org
Subject: Re: [openstack-dev] [tc][all] A culture change (nitpicking)

> "master should be always deployable and fully backward compatible and
> so we cant let anything in anytime that could possibly regress anyone"
>
> Should we change that attitude too? Anyone agree? disagree?
>
> Thanks,
> Dims
>
I'll definitely jump at this one.

I've always thought (and shared on the ML several times now) that our
implied
but not explicit support for CD from any random commit was a bad thing.

While I think it's good to support the idea that master is always
deployable, I
do not think it is a good mindset to think that every commit is a
"release" and
therefore should be supported until the end of time. We have a coordinated
release for a reason, and I think design decisions and fixes should be
based on
the assumption that a release is a release and the point at which we
need to be
cognizant and caring about keeping backward compatibility. Doing that for
every single commit is not ideal for the overall health of the product, IMO.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Sean McGinnis]

"master should be always deployable and fully backward compatible and
so we cant let anything in anytime that could possibly regress anyone"

Should we change that attitude too? Anyone agree? disagree?

Thanks,
Dims


I'll definitely jump at this one.

I've always thought (and shared on the ML several times now) that our 
implied

but not explicit support for CD from any random commit was a bad thing.

While I think it's good to support the idea that master is always 
deployable, I
do not think it is a good mindset to think that every commit is a 
"release" and

therefore should be supported until the end of time. We have a coordinated
release for a reason, and I think design decisions and fixes should be 
based on
the assumption that a release is a release and the point at which we 
need to be

cognizant and caring about keeping backward compatibility. Doing that for
every single commit is not ideal for the overall health of the product, IMO.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Amy Marrich]

Coming from Ops, yes things should always be deployable, backward
compatible and shouldn't break, but at the same time we're talking about a
master branch which is always in flux and not an actual release. I think
that statement you provided Dims should apply to releases or tags and not
the master branch as a whole. And to be honest unless someone desperately
needed something just committed into master, I doubt most folks are using
master in dev let alone production, at least that's my hope.

We can't move forward as a community if we don't welcome new members to it,
which is the heart of this proposal.

Amy (spotz)

On Wed, May 30, 2018 at 2:50 PM, Davanum Srinivas  wrote:

> Please see below:
>
> On Wed, May 30, 2018 at 2:37 PM, Chris Dent 
> wrote:
> > On Wed, 30 May 2018, Julia Kreger wrote:
> >
> >> I don't feel like anyone is proposing to end the use of -1's, but that
> >> we should generally be encouraging, accepting, and trusting.
> >
> >
> > Being encouraging, accepting, and trusting is the outcome I'd like
> > to see from this process. Being less nitpicking is a behavior or
> > process change. Adjusting attitudes (in some environments lightly,
> > in others more) so that we (where "we" is regulars in the projects,
> > experienced reviewers, and cores) perceive patches as something to
> > be grateful for and shepherded instead of an intrusion or obligation
> > would be a significant and beneficial culture change.
> >
> > A perhaps more straightforward way to put it is: When someone (even
> > one of "us") submits a patch they are doing us (the same "we" as
> > above) a favor and we owe them not just a cordial and supportive
> > response, but one with some continuity.
> >
> > Like many, I'm guilty of letting a false or inflated sense of urgency
> > get the better of me and being an ass in some reviews. Sorry about
> > that.
> >
> > A cultural shift in this area will improve things for all of us.
> > Nitpicking is symptomatic of an attitude, one we can change, not the
> > disease itself.
> >
> >> We also need to be mindful
> >> of context as well, and in the grand scheme not try for something
> >> perfect as many often do. This *does* mean we land something that
> >> needs to be fixed later or reverted later, but neither are things we
> >> should fear. We can't let that fear control us.
>
> Let me poke at this a bit. Some of the projects do say (not in so many
> words):
>
> "master should be always deployable and fully backward compatible and
> so we cant let anything in anytime that could possibly regress anyone"
>
> Should we change that attitude too? Anyone agree? disagree?
>
> Thanks,
> Dims
>
> >
> > Yes, very much yes.
> >
> > --
> > Chris Dent   ٩◔̯◔۶   https://anticdent.org/
> > freenode: cdent tw: @anticdent
> > 
> __
> > OpenStack Development Mailing List (not for usage questions)
> > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:
> unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
>
>
>
> --
> Davanum Srinivas :: https://twitter.com/dims
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Davanum Srinivas]

Please see below:

On Wed, May 30, 2018 at 2:37 PM, Chris Dent  wrote:
> On Wed, 30 May 2018, Julia Kreger wrote:
>
>> I don't feel like anyone is proposing to end the use of -1's, but that
>> we should generally be encouraging, accepting, and trusting.
>
>
> Being encouraging, accepting, and trusting is the outcome I'd like
> to see from this process. Being less nitpicking is a behavior or
> process change. Adjusting attitudes (in some environments lightly,
> in others more) so that we (where "we" is regulars in the projects,
> experienced reviewers, and cores) perceive patches as something to
> be grateful for and shepherded instead of an intrusion or obligation
> would be a significant and beneficial culture change.
>
> A perhaps more straightforward way to put it is: When someone (even
> one of "us") submits a patch they are doing us (the same "we" as
> above) a favor and we owe them not just a cordial and supportive
> response, but one with some continuity.
>
> Like many, I'm guilty of letting a false or inflated sense of urgency
> get the better of me and being an ass in some reviews. Sorry about
> that.
>
> A cultural shift in this area will improve things for all of us.
> Nitpicking is symptomatic of an attitude, one we can change, not the
> disease itself.
>
>> We also need to be mindful
>> of context as well, and in the grand scheme not try for something
>> perfect as many often do. This *does* mean we land something that
>> needs to be fixed later or reverted later, but neither are things we
>> should fear. We can't let that fear control us.

Let me poke at this a bit. Some of the projects do say (not in so many words):

"master should be always deployable and fully backward compatible and
so we cant let anything in anytime that could possibly regress anyone"

Should we change that attitude too? Anyone agree? disagree?

Thanks,
Dims

>
> Yes, very much yes.
>
> --
> Chris Dent   ٩◔̯◔۶   https://anticdent.org/
> freenode: cdent tw: @anticdent
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 
Davanum Srinivas :: https://twitter.com/dims

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Chris Dent]

On Wed, 30 May 2018, Julia Kreger wrote:


I don't feel like anyone is proposing to end the use of -1's, but that
we should generally be encouraging, accepting, and trusting.


Being encouraging, accepting, and trusting is the outcome I'd like
to see from this process. Being less nitpicking is a behavior or
process change. Adjusting attitudes (in some environments lightly,
in others more) so that we (where "we" is regulars in the projects,
experienced reviewers, and cores) perceive patches as something to
be grateful for and shepherded instead of an intrusion or obligation
would be a significant and beneficial culture change.

A perhaps more straightforward way to put it is: When someone (even
one of "us") submits a patch they are doing us (the same "we" as
above) a favor and we owe them not just a cordial and supportive
response, but one with some continuity.

Like many, I'm guilty of letting a false or inflated sense of urgency
get the better of me and being an ass in some reviews. Sorry about
that.

A cultural shift in this area will improve things for all of us.
Nitpicking is symptomatic of an attitude, one we can change, not the
disease itself.


We also need to be mindful
of context as well, and in the grand scheme not try for something
perfect as many often do. This *does* mean we land something that
needs to be fixed later or reverted later, but neither are things we
should fear. We can't let that fear control us.


Yes, very much yes.

--
Chris Dent   ٩◔̯◔۶   https://anticdent.org/
freenode: cdent tw: @anticdent__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Clark Boylan]

On Wed, May 30, 2018, at 8:13 AM, Zane Bitter wrote:
> On 30/05/18 00:52, Cédric Jeanneret wrote:
> >> Another issue is that if the original author needs to rev the patch
> >> again for any reason, they then need to figure out how to check out the
> >> modified patch. This requires a fairly sophisticated knowledge of both
> >> git and gerrit, which isn't a problem for those of us who have been
> >> using them for years but is potentially a nightmarish introduction for a
> >> relatively new contributor. Sometimes it's the right choice though
> >> (especially if the patch owner hasn't been seen for a while).
> > hm, "Download" -> copy/paste, and Voilà. Gerrit interface is pretty nice
> > with the user (I an "old new contributor", never really struggled with
> > Gerrit itself.. On the other hand, heat, ansible, that's another story :) ).
> 
> OK, so I am sitting here with a branch containing a patch I have sent 
> for review, and that I need to revise, but somebody else has pushed a 
> revision upstream. Which of the 4 'Download' commands do I use to 
> replace my commit with the latest one from upstream?
> 
> (Hint: it's a trick question)
> 
> Now imagine it wasn't the last patch in the series.
> 
> - ZB
> 
> (P.S. without testing, I believe the correct answers are `git reset 
> --hard FETCH_HEAD` and `git rebase HEAD~ --onto FETCH_HEAD` 
> respectively.)

We do have tools for this and it is the same tool you use to push code to 
gerrit. `git review -d changenumber` will grab the latest patchset from that 
change and check it out locally. You can use `git review -d 
changenumber,patchsetnumber` to pick a different older patchset. If you have a 
series of changes things become more complicated. I personally like to always 
operate against leaf most change, make local updates, then squash "back" onto 
the appropriate changes in the series to keep existing changes happy.

Yes, this is complicated especially for new users. In general though I think 
git review addresses the simpler cases of I need to use latest version of my 
change. If we use change series as proposed in this thread I think keeping the 
parent of the child changes up to date is going to be up to the more 
experienced nit picker that is addressing the minor problems and not the 
original change author.

Clark

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Luigi Toscano]

On Wednesday, 30 May 2018 17:13:41 CEST Zane Bitter wrote:
> On 30/05/18 00:52, Cédric Jeanneret wrote:
> >> Another issue is that if the original author needs to rev the patch
> >> again for any reason, they then need to figure out how to check out the
> >> modified patch. This requires a fairly sophisticated knowledge of both
> >> git and gerrit, which isn't a problem for those of us who have been
> >> using them for years but is potentially a nightmarish introduction for a
> >> relatively new contributor. Sometimes it's the right choice though
> >> (especially if the patch owner hasn't been seen for a while).
> > 
> > hm, "Download" -> copy/paste, and Voilà. Gerrit interface is pretty nice
> > with the user (I an "old new contributor", never really struggled with
> > Gerrit itself.. On the other hand, heat, ansible, that's another story :)
> > ).
> OK, so I am sitting here with a branch containing a patch I have sent
> for review, and that I need to revise, but somebody else has pushed a
> revision upstream. Which of the 4 'Download' commands do I use to
> replace my commit with the latest one from upstream?
> 
> (Hint: it's a trick question)
> 
> Now imagine it wasn't the last patch in the series.

Maybe using git review is enough:

git review -d 

We document git review -d for rebasing and adding new review on top of a 
existing reviews, but not explicitly for amending a change:
https://docs.openstack.org/infra/manual/developers.html

Ciao
-- 
Luigi



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 30/05/18 00:52, Cédric Jeanneret wrote:

Another issue is that if the original author needs to rev the patch
again for any reason, they then need to figure out how to check out the
modified patch. This requires a fairly sophisticated knowledge of both
git and gerrit, which isn't a problem for those of us who have been
using them for years but is potentially a nightmarish introduction for a
relatively new contributor. Sometimes it's the right choice though
(especially if the patch owner hasn't been seen for a while).

hm, "Download" -> copy/paste, and Voilà. Gerrit interface is pretty nice
with the user (I an "old new contributor", never really struggled with
Gerrit itself.. On the other hand, heat, ansible, that's another story :) ).


OK, so I am sitting here with a branch containing a patch I have sent 
for review, and that I need to revise, but somebody else has pushed a 
revision upstream. Which of the 4 'Download' commands do I use to 
replace my commit with the latest one from upstream?


(Hint: it's a trick question)

Now imagine it wasn't the last patch in the series.

- ZB

(P.S. without testing, I believe the correct answers are `git reset 
--hard FETCH_HEAD` and `git rebase HEAD~ --onto FETCH_HEAD` 
respectively.)


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Yolanda Robla Mota]

I see another problem working on patchsets with lots of revisions and
long-lived history, such as specs or a complex change. The opinions of
several reviewers may be different. So first reviewer lefts a comment, the
owner of the change amends the patch according to it. But after time and
iterations pass (and because the history is very long or impossible to
read) another reviewer who has not read the whole history, may come and put
a -1 that contradicts the initial change. Things like that could be sorted
if we keep shorter patchsets, and merge things that are production ready
but not perfect, then we come up with follow-up patches to amend possible
comments, or create enhancements.

On Wed, May 30, 2018 at 4:16 PM, Dmitry Tantsur  wrote:

> On 05/30/2018 03:54 PM, Julia Kreger wrote:
>
>> On Tue, May 29, 2018 at 7:42 PM, Zane Bitter  wrote:
>> [trim]
>>
>>> Since I am replying to this thread, Julia also mentioned the situation
>>> where
>>> two core reviewers are asking for opposite changes to a patch. It is
>>> never
>>> ever ever the contributor's responsibility to resolve a dispute between
>>> two
>>> core reviewers! If you see a core reviewer's advice on a patch and you
>>> want
>>> to give the opposite advice, by all means take it up immediately - with
>>> *the
>>> other core reviewer*. NOT the submitter. Preferably on IRC and not in the
>>> review. You work together every day, you can figure it out! A random
>>> contributor has no chance of parachuting into the middle of that dynamic
>>> and
>>> walking out unscathed, and they should never be asked to.
>>>
>>>
>> Absolutely agree! In the case that was in mind where it has happened
>> to me personally, I think it was 10-15 revisions apart, so it becomes
>> a little hard to identify when your starting to play the game of "make
>> the cores happy to land it". It is not a fun game for the contributor.
>> Truthfully it caused me to add the behavior of intentionally waiting
>> longer between uploads of new revisions... which does not help at all.
>>
>> The other conundrum is when you disagree and the person has left a -1
>> which blocks forward progress and any additional reviews since it gets
>> viewed as "not ready", which makes it even harder and slower to build
>> consensus. At some point you get into "Oh, what formatting can I
>> change to clear that -1 because the person is not responding" mode.
>>
>
> This, by the way, is a much broader and interesting question. In case of a
> by-passer leaving a comment ("this link must be https") and disappearing,
> the PTL or any core can remove the reviewer from the review. What to do
> with a core leaving a comment or non-core leaving a potentially useful
> comment and going to PTO?
>
>
>
>> At least beginning to shift the review culture should help many of these
>> issues.
>>
>> 
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscrib
>> e
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>



-- 

Yolanda Robla Mota

Principal Software Engineer, RHCE

Red Hat



C/Avellana 213

Urb Portugal

yrobl...@redhat.comM: +34605641639


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Julia Kreger]

I don't feel like anyone is proposing to end the use of -1's, but that
we should generally be encouraging, accepting, and trusting. That
means if there are major gaps or issues, then the use of a -1 is
perfectly valid because it needs more work. We also need to be mindful
of context as well, and in the grand scheme not try for something
perfect as many often do. This *does* mean we land something that
needs to be fixed later or reverted later, but neither are things we
should fear. We can't let that fear control us.

There are also the qualifiers of "does this help" or "does this harm",
and neither are nitpicks to me. That is also going to vary from
project to project, and even more so case by case based upon the item
being evaluated. That is where the context is important. Perhaps we
need to also, as the community evolves, consider mindfulness. Granted,
mindfulness is hard and can be even harder with elevated stress. Maybe
this is where we should also pitch something like "Stacker Cruises"
with an intermediate forced vacation for everyone. :)

On Wed, May 30, 2018 at 6:11 AM, Dmitry Tantsur  wrote:
> Hi,
>
> This is a great discussion and a great suggestion overall, but I'd like to
> add a grain of salt here, especially after reading some comments.
>
> Nitpicking is bad, no disagreement. However, I don't like this whole
> discussion to end up marking -1's as offense or aggression. Just as often as
> I see newcomers proposing patches frustrated with many iterations, I see
> newcomers being afraid to -1.
>
> In my personal experience I have two remarkable cases:
> 1. A person asking me (via a private message) to not put -1 on their patches
> because they may have problems with their managers.
> 2. A person proposing a follow-up on *any* comment to their patch, including
> important ones.
>
> Whatever decision the TC takes, I would like it to make sure that we don't
> paint putting -1 as a bad act. Nor do I want "if you care, just follow-up"
> to be an excuse for putting up bad contributions.
>
> Additionally, I would like to have something saying that a -1 is valid and
> appropriate, if a contribution substantially increases the project's
> technical debt. After already spending *days* refactoring ironic unit tests,
> I will -1 the hell out of a patch that will try to bring them back to their
> initial state, I promise :)
>
> Dmitry
>
>
> On 05/29/2018 03:55 PM, Julia Kreger wrote:
>>
>> During the Forum, the topic of review culture came up in session after
>> session. During these discussions, the subject of our use of nitpicks
>> were often raised as a point of contention and frustration, especially
>> by community members that have left the community and that were
>> attempting to re-engage the community. Contributors raised the point
>> of review feedback requiring for extremely precise English, or
>> compliance to a particular core reviewer's style preferences, which
>> may not be the same as another core reviewer.
>>
>> These things are not just frustrating, but also very inhibiting for
>> part time contributors such as students who may also be time limited.
>> Or an operator who noticed something that was clearly a bug and that
>> put forth a very minor fix and doesn't have the time to revise it over
>> and over.
>>
>> While nitpicks do help guide and teach, the consensus seemed to be
>> that we do need to shift the culture a little bit. As such, I've
>> proposed a change to our principles[1] in governance that attempts to
>> capture the essence and spirit of the nitpicking topic as a first
>> step.
>>
>> -Julia
>> -
>> [1]: https://review.openstack.org/570940
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Dmitry Tantsur]

On 05/30/2018 03:54 PM, Julia Kreger wrote:

On Tue, May 29, 2018 at 7:42 PM, Zane Bitter  wrote:
[trim]

Since I am replying to this thread, Julia also mentioned the situation where
two core reviewers are asking for opposite changes to a patch. It is never
ever ever the contributor's responsibility to resolve a dispute between two
core reviewers! If you see a core reviewer's advice on a patch and you want
to give the opposite advice, by all means take it up immediately - with *the
other core reviewer*. NOT the submitter. Preferably on IRC and not in the
review. You work together every day, you can figure it out! A random
contributor has no chance of parachuting into the middle of that dynamic and
walking out unscathed, and they should never be asked to.



Absolutely agree! In the case that was in mind where it has happened
to me personally, I think it was 10-15 revisions apart, so it becomes
a little hard to identify when your starting to play the game of "make
the cores happy to land it". It is not a fun game for the contributor.
Truthfully it caused me to add the behavior of intentionally waiting
longer between uploads of new revisions... which does not help at all.

The other conundrum is when you disagree and the person has left a -1
which blocks forward progress and any additional reviews since it gets
viewed as "not ready", which makes it even harder and slower to build
consensus. At some point you get into "Oh, what formatting can I
change to clear that -1 because the person is not responding" mode.


This, by the way, is a much broader and interesting question. In case of a 
by-passer leaving a comment ("this link must be https") and disappearing, the 
PTL or any core can remove the reviewer from the review. What to do with a core 
leaving a comment or non-core leaving a potentially useful comment and going to PTO?




At least beginning to shift the review culture should help many of these issues.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Julia Kreger]

On Tue, May 29, 2018 at 7:42 PM, Zane Bitter  wrote:
[trim]
> Since I am replying to this thread, Julia also mentioned the situation where
> two core reviewers are asking for opposite changes to a patch. It is never
> ever ever the contributor's responsibility to resolve a dispute between two
> core reviewers! If you see a core reviewer's advice on a patch and you want
> to give the opposite advice, by all means take it up immediately - with *the
> other core reviewer*. NOT the submitter. Preferably on IRC and not in the
> review. You work together every day, you can figure it out! A random
> contributor has no chance of parachuting into the middle of that dynamic and
> walking out unscathed, and they should never be asked to.
>

Absolutely agree! In the case that was in mind where it has happened
to me personally, I think it was 10-15 revisions apart, so it becomes
a little hard to identify when your starting to play the game of "make
the cores happy to land it". It is not a fun game for the contributor.
Truthfully it caused me to add the behavior of intentionally waiting
longer between uploads of new revisions... which does not help at all.

The other conundrum is when you disagree and the person has left a -1
which blocks forward progress and any additional reviews since it gets
viewed as "not ready", which makes it even harder and slower to build
consensus. At some point you get into "Oh, what formatting can I
change to clear that -1 because the person is not responding" mode.

At least beginning to shift the review culture should help many of these issues.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Thierry Carrez]

Jay S Bryant wrote:


On 5/29/2018 3:19 PM, Doug Hellmann wrote:

Excerpts from Jonathan Proulx's message of 2018-05-29 16:05:06 -0400:

On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
:> >> maybe we're all saying the same thing here?
:> > Yeah, I feel like we're all essentially in agreement that nits 
(of the

:> > English mistake of typo type) do need to get fixed, but sometimes
:> > (often?) putting the burden of fixing them on the original patch
:> > contributor is neither fair nor constructive.
:> I am ok with this statement if we are all in agreement that doing
:> follow-up patches is an acceptable practice.
:
:Has it ever not been?
:
:It seems like it has always come down to a bit of negotiation with
:the original author, hasn't it? And that won't change, except that
:we will be emphasizing to reviewers that we encourage them to be
:more active in seeking out that negotiation and then proposing
:patches?

Exactly, it's more codifying a default.

It's not been unacceptable but I think there's some understandable
reluctance to make changes to someone else's work, you don't want to
seem like your taking over or getting in the way.  At least that's
what's in my head when deciding should this be a comment or a patch.

I think this discussion suggests for certain class of "nits" patch is
preferred to comment.  If that is true making this explicit is a good
thing becuase let's face it my social skills are only marginally
better than my speeling :)

-Jon


OK, that's all good. I'm just surprised to learn that throwing a
follow-up patch on top of someone else's patch was ever seen as
discouraged.

The spice must flow,
Doug


Maybe it would be different now that I am a Core/PTL but in the past I 
had been warned to be careful as it could be misinterpreted if I was 
changing other people's patches or that it could look like I was trying 
to pad my numbers. (I am a nit-picker though I do my best not to be.


There still seems to be some confusion between "new patchset over 
existing change" and "follow-up separate change" as to what is the 
recommended way to fix nits.


To clarify, the proposal here is to encourage the posting of a follow-up 
change.


--
Thierry Carrez (ttx)

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Dmitry Tantsur]

Hi,

This is a great discussion and a great suggestion overall, but I'd like to add a 
grain of salt here, especially after reading some comments.


Nitpicking is bad, no disagreement. However, I don't like this whole discussion 
to end up marking -1's as offense or aggression. Just as often as I see 
newcomers proposing patches frustrated with many iterations, I see newcomers 
being afraid to -1.


In my personal experience I have two remarkable cases:
1. A person asking me (via a private message) to not put -1 on their patches 
because they may have problems with their managers.
2. A person proposing a follow-up on *any* comment to their patch, including 
important ones.


Whatever decision the TC takes, I would like it to make sure that we don't paint 
putting -1 as a bad act. Nor do I want "if you care, just follow-up" to be an 
excuse for putting up bad contributions.


Additionally, I would like to have something saying that a -1 is valid and 
appropriate, if a contribution substantially increases the project's technical 
debt. After already spending *days* refactoring ironic unit tests, I will -1 the 
hell out of a patch that will try to bring them back to their initial state, I 
promise :)


Dmitry

On 05/29/2018 03:55 PM, Julia Kreger wrote:

During the Forum, the topic of review culture came up in session after
session. During these discussions, the subject of our use of nitpicks
were often raised as a point of contention and frustration, especially
by community members that have left the community and that were
attempting to re-engage the community. Contributors raised the point
of review feedback requiring for extremely precise English, or
compliance to a particular core reviewer's style preferences, which
may not be the same as another core reviewer.

These things are not just frustrating, but also very inhibiting for
part time contributors such as students who may also be time limited.
Or an operator who noticed something that was clearly a bug and that
put forth a very minor fix and doesn't have the time to revise it over
and over.

While nitpicks do help guide and teach, the consensus seemed to be
that we do need to shift the culture a little bit. As such, I've
proposed a change to our principles[1] in governance that attempts to
capture the essence and spirit of the nitpicking topic as a first
step.

-Julia
-
[1]: https://review.openstack.org/570940

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Cédric Jeanneret]

On 05/29/2018 09:43 PM, Davanum Srinivas wrote:
> Agree with Ian here.
> 
> Also another problem that comes up is: "Why are you touching *MY*
> review?" (probably coming from the view where stats - and stackalytics
> leaderboard position is important). So i guess we ask permission
> before editing (or) file a follow up later (or) just tell folks that
> this is ok to do!!

We call that "communication". It's an important thing, especially in an
open source project with many, many contributors from many, many
different cultures and languages. Good communication avoids wars ;).

So yep - if someone has anything to correct and feels the urge to push a
patch on someone else', please do, but communicate the reasons (at
least, that's my point of view).

Cheers,

C.

> 
> Hoping engaging with them will solve yet another issue is someone
> going around filing the same change in a dozen projects (repeatedly!),
> but that may be wishful thinking.
> 
> -- Dims
> 
> On Tue, May 29, 2018 at 12:17 PM, Ian Wells  wrote:
>> If your nitpick is a spelling mistake or the need for a comment where you've
>> pretty much typed the text of the comment in the review comment itself, then
>> I have personally found it easiest to use the Gerrit online editor to
>> actually update the patch yourself.  There's nothing magical about the
>> original submitter, and no point in wasting your time and theirs to get them
>> to make the change.  That said, please be a grown up; if you're changing
>> code or messing up formatting enough for PEP8 to be a concern, it's your
>> responsibility, not the original submitter's, to fix it.  Also, do all your
>> fixes in one commit if you don't want to make Zuul cry.
>> --
>> Ian.
>>
>>
>> On 29 May 2018 at 09:00, Neil Jerram  wrote:
>>>
>>> From my point of view as someone who is still just an occasional
>>> contributor (in all OpenStack projects other than my own team's networking
>>> driver), and so I think still sensitive to the concerns being raised here:
>>>
>>> - Nits are not actually a problem, at all, if they are uncontroversial and
>>> quick to deal with.  For example, if it's a point of English, and most
>>> English speakers would agree that a correction is better, it's quick and no
>>> problem for me to make that correction.
>>>
>>> - What is much more of a problem is:
>>>
>>>   - Anything that is more a matter of opinion.  If a markup is just the
>>> reviewer's personal opinion, and they can't say anything to explain more
>>> objectively why their suggestion is better, it would be wiser to defer to
>>> the contributor's initial choice.
>>>
>>>   - Questioning something unconstructively or out of proportion to the
>>> change being made.  This is a tricky one to pin down, but sometimes I've had
>>> comments that raise some random left-field question that isn't really
>>> related to the change being made, or where the reviewer could have done a
>>> couple minutes research themselves and then either made a more precise
>>> comment, or not made their comment at all.
>>>
>>>   - Asking - implicitly or explicitly - the contributor to add more
>>> cleanups to their change.  If someone usefully fixes a problem, and their
>>> fix does not of itself impair the quality or maintainability of the
>>> surrounding code, they should not be asked to extend their fix so as to fix
>>> further problems that a more regular developer may be aware of in that area,
>>> or to advance a refactoring / cleanup that another developer has in mind.
>>> (At least, not as part of that initial change.)
>>>
>>> (Obviously the common thread of those problem points is taking up more
>>> time; psychologically I think one of the things that can turn a contributor
>>> away is the feeling that they've contributed a clearly useful thing, yet the
>>> community is stalling over accepting it for reasons that do not appear
>>> clearcut.)
>>>
>>> Hoping this is vaguely helpful...
>>>  Neil
>>>
>>>
>>> On Tue, May 29, 2018 at 4:35 PM Amy Marrich  wrote:

 If I have a nit that doesn't affect things, I'll make a note of it and
 say if you do another patch I'd really like it fixed but also give the 
 patch
 a vote. What I'll also do sometimes if I know the user or they are online
 I'll offer to fix things for them, that way they can see what I've done,
 I've sped things along and I haven't caused a simple change to take a long
 amount of time and reviews.

 I think this is a great addition!

 Thanks,

 Amy (spotz)

 On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
  wrote:
>
> During the Forum, the topic of review culture came up in session after
> session. During these discussions, the subject of our use of nitpicks
> were often raised as a point of contention and frustration, especially
> by community members that have left the community and that were
> attempting to re-engage the community. Contributors raised the point
> of review feedback 

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Cédric Jeanneret]

On 05/30/2018 01:42 AM, Zane Bitter wrote:
> On 29/05/18 16:49, Slawomir Kaplonski wrote:
>> Hi,
>>
>>> Wiadomość napisana przez Jay S Bryant  w dniu
>>> 29.05.2018, o godz. 22:25:
>>> Maybe it would be different now that I am a Core/PTL but in the past
>>> I had been warned to be careful as it could be misinterpreted if I
>>> was changing other people's patches or that it could look like I was
>>> trying to pad my numbers. (I am a nit-picker though I do my best not
>>> to be.
>>
>> Exactly. I remember when I was doing my first patch (or one of first
>> patches) and someone pushed new PS with some very small nits fixed. I
>> was a bit confused because of that and I was thinking why he did it
>> instead of me?
>> Now it’s of course much more clear for me but for someone who is new
>> contributor I think that this might be confusing. Maybe such person
>> should at least remember to explain in comment why he pushed new PS
>> and that’s not „stealing” work of original author :)
> 
> Another issue is that if the original author needs to rev the patch
> again for any reason, they then need to figure out how to check out the
> modified patch. This requires a fairly sophisticated knowledge of both
> git and gerrit, which isn't a problem for those of us who have been
> using them for years but is potentially a nightmarish introduction for a
> relatively new contributor. Sometimes it's the right choice though
> (especially if the patch owner hasn't been seen for a while).

hm, "Download" -> copy/paste, and Voilà. Gerrit interface is pretty nice
with the user (I an "old new contributor", never really struggled with
Gerrit itself.. On the other hand, heat, ansible, that's another story :) ).

> 
> A follow-up patch is a good alternative, unless of course it conflicts
> with another patch in the series.
> 
> +1 with a comment can also get you a long way - it indicates that you've
> reviewed the whole patch and have found only nits to quibble with. If
> you're a core reviewer, another core could potentially +2/+A on a
> subsequent patch set with the nits addressed if they felt it
> appropriate, and even if they don't you'll have an easy re-review when
> you follow up.
> 
> We have lots of tools in our toolbox that are less blunt than -1. Let's
> save -1 for when major work is required and/or the patch as written
> would actually break something.

+1 (not core, can't +2, sorry :D)

"-1" is "aggressive".

Cheers,

C.

> 
> 
> Since I am replying to this thread, Julia also mentioned the situation
> where two core reviewers are asking for opposite changes to a patch. It
> is never ever ever the contributor's responsibility to resolve a dispute
> between two core reviewers! If you see a core reviewer's advice on a
> patch and you want to give the opposite advice, by all means take it up
> immediately - with *the other core reviewer*. NOT the submitter.
> Preferably on IRC and not in the review. You work together every day,
> you can figure it out! A random contributor has no chance of parachuting
> into the middle of that dynamic and walking out unscathed, and they
> should never be asked to.
> 
> cheers,
> Zane.
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 
Cédric Jeanneret
Software Engineer
DFG:DF



signature.asc
Description: OpenPGP digital signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Masayuki Igawa]

I think this is not only for code but also for doc and reno. We should
fix it basically, especially about doc/reno. But I don't think it
should be in the same patch if the mistake isn't critical which means
*nitpicks*. I think we can fix them with following patches if we need
to fix it.

Otherwise, writing reno/doc might be a really difficult and hard work
for ESL people like me.

-- Masayuki


On 05/30, Ghanshyam Mann wrote:
> Thanks for making it formal process which really helps. I think most
> of the people usually does that but yes it is always helpful to be
> added as principles.
> 
> I have gotten mix feedback on fixing other patches in past and when i
> got anger by author i try to leave comment for a day or two then fix
> if it is really needed otherwise just go with follow-up.
> 
> One question - This only applies to code nitpick only right? not
> documentation or releasenotes. For doc and reno, we should fix spell
> or grammar mistake etc in same patch.
> 
> -gmann
> 
> 
> On Tue, May 29, 2018 at 10:55 PM, Julia Kreger
>  wrote:
> > During the Forum, the topic of review culture came up in session after
> > session. During these discussions, the subject of our use of nitpicks
> > were often raised as a point of contention and frustration, especially
> > by community members that have left the community and that were
> > attempting to re-engage the community. Contributors raised the point
> > of review feedback requiring for extremely precise English, or
> > compliance to a particular core reviewer's style preferences, which
> > may not be the same as another core reviewer.
> >
> > These things are not just frustrating, but also very inhibiting for
> > part time contributors such as students who may also be time limited.
> > Or an operator who noticed something that was clearly a bug and that
> > put forth a very minor fix and doesn't have the time to revise it over
> > and over.
> >
> > While nitpicks do help guide and teach, the consensus seemed to be
> > that we do need to shift the culture a little bit. As such, I've
> > proposed a change to our principles[1] in governance that attempts to
> > capture the essence and spirit of the nitpicking topic as a first
> > step.
> >
> > -Julia
> > -
> > [1]: https://review.openstack.org/570940
> >
> > __
> > OpenStack Development Mailing List (not for usage questions)
> > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 
Happy Hacking!!
  Masayuki Igawa
  GPG fingerprint = C27C 2F00 3A2A 999A 903A  753D 290F 53ED C899 BF90


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Ghanshyam Mann]

Thanks for making it formal process which really helps. I think most
of the people usually does that but yes it is always helpful to be
added as principles.

I have gotten mix feedback on fixing other patches in past and when i
got anger by author i try to leave comment for a day or two then fix
if it is really needed otherwise just go with follow-up.

One question - This only applies to code nitpick only right? not
documentation or releasenotes. For doc and reno, we should fix spell
or grammar mistake etc in same patch.

-gmann


On Tue, May 29, 2018 at 10:55 PM, Julia Kreger
 wrote:
> During the Forum, the topic of review culture came up in session after
> session. During these discussions, the subject of our use of nitpicks
> were often raised as a point of contention and frustration, especially
> by community members that have left the community and that were
> attempting to re-engage the community. Contributors raised the point
> of review feedback requiring for extremely precise English, or
> compliance to a particular core reviewer's style preferences, which
> may not be the same as another core reviewer.
>
> These things are not just frustrating, but also very inhibiting for
> part time contributors such as students who may also be time limited.
> Or an operator who noticed something that was clearly a bug and that
> put forth a very minor fix and doesn't have the time to revise it over
> and over.
>
> While nitpicks do help guide and teach, the consensus seemed to be
> that we do need to shift the culture a little bit. As such, I've
> proposed a change to our principles[1] in governance that attempts to
> capture the essence and spirit of the nitpicking topic as a first
> step.
>
> -Julia
> -
> [1]: https://review.openstack.org/570940
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Samuel Cassiba]

On Tue, May 29, 2018 at 4:26 PM, Ian Wells  wrote:

> On 29 May 2018 at 14:53, Jeremy Stanley  wrote:
>
>> On 2018-05-29 15:25:01 -0500 (-0500), Jay S Bryant wrote:
>> [...]
>> > Maybe it would be different now that I am a Core/PTL but in the past I
>> had
>> > been warned to be careful as it could be misinterpreted if I was
>> changing
>> > other people's patches or that it could look like I was trying to pad my
>> > numbers. (I am a nit-picker though I do my best not to be.
>> [...]
>>
>> Most stats tracking goes by the Gerrit "Owner" metadata or the Git
>> "Author" field, neither of which are modified in a typical new
>> patchset workflow and so carry over from the original patchset #1
>> (resetting Author requires creating a new commit from scratch or
>> passing extra options to git to reset it, while changing the Owner
>> needs a completely new Change-Id footer).
>>
>
> We know this, but other people don't, so the comment is wise.  Also,
> arguably, if I badly fix someone else's patch, I'm making them look bad by
> leaving them with the 'credit' for my bad work, so it's important to be
> careful and tactful.  But the history is public record, at least.
>
>
If the patch is bad enough where I have to step in to rewrite, I'm making
the submitter look bad no matter what. That makes everyone worse off.

Best,
Samuel


> --
> Ian.
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Ben Swartzlander]

On 05/29/2018 03:43 PM, Davanum Srinivas wrote:

Agree with Ian here.

Also another problem that comes up is: "Why are you touching *MY*
review?" (probably coming from the view where stats - and stackalytics
leaderboard position is important). So i guess we ask permission
before editing (or) file a follow up later (or) just tell folks that
this is ok to do!!


I think Stackalytics is evil and should be killed with fire. It 
encourages all kinds of pathological behavior, this being one prime 
example. Having worked as a core reviewer, I find zero value from the 
project. We know who is contributing code and who is doing reviews 
without some robot to tell us.


-Ben



Hoping engaging with them will solve yet another issue is someone
going around filing the same change in a dozen projects (repeatedly!),
but that may be wishful thinking.

-- Dims

On Tue, May 29, 2018 at 12:17 PM, Ian Wells  wrote:

If your nitpick is a spelling mistake or the need for a comment where you've
pretty much typed the text of the comment in the review comment itself, then
I have personally found it easiest to use the Gerrit online editor to
actually update the patch yourself.  There's nothing magical about the
original submitter, and no point in wasting your time and theirs to get them
to make the change.  That said, please be a grown up; if you're changing
code or messing up formatting enough for PEP8 to be a concern, it's your
responsibility, not the original submitter's, to fix it.  Also, do all your
fixes in one commit if you don't want to make Zuul cry.
--
Ian.


On 29 May 2018 at 09:00, Neil Jerram  wrote:


 From my point of view as someone who is still just an occasional
contributor (in all OpenStack projects other than my own team's networking
driver), and so I think still sensitive to the concerns being raised here:

- Nits are not actually a problem, at all, if they are uncontroversial and
quick to deal with.  For example, if it's a point of English, and most
English speakers would agree that a correction is better, it's quick and no
problem for me to make that correction.

- What is much more of a problem is:

   - Anything that is more a matter of opinion.  If a markup is just the
reviewer's personal opinion, and they can't say anything to explain more
objectively why their suggestion is better, it would be wiser to defer to
the contributor's initial choice.

   - Questioning something unconstructively or out of proportion to the
change being made.  This is a tricky one to pin down, but sometimes I've had
comments that raise some random left-field question that isn't really
related to the change being made, or where the reviewer could have done a
couple minutes research themselves and then either made a more precise
comment, or not made their comment at all.

   - Asking - implicitly or explicitly - the contributor to add more
cleanups to their change.  If someone usefully fixes a problem, and their
fix does not of itself impair the quality or maintainability of the
surrounding code, they should not be asked to extend their fix so as to fix
further problems that a more regular developer may be aware of in that area,
or to advance a refactoring / cleanup that another developer has in mind.
(At least, not as part of that initial change.)

(Obviously the common thread of those problem points is taking up more
time; psychologically I think one of the things that can turn a contributor
away is the feeling that they've contributed a clearly useful thing, yet the
community is stalling over accepting it for reasons that do not appear
clearcut.)

Hoping this is vaguely helpful...
  Neil


On Tue, May 29, 2018 at 4:35 PM Amy Marrich  wrote:


If I have a nit that doesn't affect things, I'll make a note of it and
say if you do another patch I'd really like it fixed but also give the patch
a vote. What I'll also do sometimes if I know the user or they are online
I'll offer to fix things for them, that way they can see what I've done,
I've sped things along and I haven't caused a simple change to take a long
amount of time and reviews.

I think this is a great addition!

Thanks,

Amy (spotz)

On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
 wrote:


During the Forum, the topic of review culture came up in session after
session. During these discussions, the subject of our use of nitpicks
were often raised as a point of contention and frustration, especially
by community members that have left the community and that were
attempting to re-engage the community. Contributors raised the point
of review feedback requiring for extremely precise English, or
compliance to a particular core reviewer's style preferences, which
may not be the same as another core reviewer.

These things are not just frustrating, but also very inhibiting for
part time contributors such as students who may also be time limited.
Or an operator who noticed something that was clearly a bug and that
put forth a very minor fix and doesn't have the time to 

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Zane Bitter]

On 29/05/18 16:49, Slawomir Kaplonski wrote:

Hi,


Wiadomość napisana przez Jay S Bryant  w dniu 29.05.2018, 
o godz. 22:25:
Maybe it would be different now that I am a Core/PTL but in the past I had been 
warned to be careful as it could be misinterpreted if I was changing other 
people's patches or that it could look like I was trying to pad my numbers. (I 
am a nit-picker though I do my best not to be.


Exactly. I remember when I was doing my first patch (or one of first patches) 
and someone pushed new PS with some very small nits fixed. I was a bit confused 
because of that and I was thinking why he did it instead of me?
Now it’s of course much more clear for me but for someone who is new 
contributor I think that this might be confusing. Maybe such person should at 
least remember to explain in comment why he pushed new PS and that’s not 
„stealing” work of original author :)


Another issue is that if the original author needs to rev the patch 
again for any reason, they then need to figure out how to check out the 
modified patch. This requires a fairly sophisticated knowledge of both 
git and gerrit, which isn't a problem for those of us who have been 
using them for years but is potentially a nightmarish introduction for a 
relatively new contributor. Sometimes it's the right choice though 
(especially if the patch owner hasn't been seen for a while).


A follow-up patch is a good alternative, unless of course it conflicts 
with another patch in the series.


+1 with a comment can also get you a long way - it indicates that you've 
reviewed the whole patch and have found only nits to quibble with. If 
you're a core reviewer, another core could potentially +2/+A on a 
subsequent patch set with the nits addressed if they felt it 
appropriate, and even if they don't you'll have an easy re-review when 
you follow up.


We have lots of tools in our toolbox that are less blunt than -1. Let's 
save -1 for when major work is required and/or the patch as written 
would actually break something.



Since I am replying to this thread, Julia also mentioned the situation 
where two core reviewers are asking for opposite changes to a patch. It 
is never ever ever the contributor's responsibility to resolve a dispute 
between two core reviewers! If you see a core reviewer's advice on a 
patch and you want to give the opposite advice, by all means take it up 
immediately - with *the other core reviewer*. NOT the submitter. 
Preferably on IRC and not in the review. You work together every day, 
you can figure it out! A random contributor has no chance of parachuting 
into the middle of that dynamic and walking out unscathed, and they 
should never be asked to.


cheers,
Zane.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Ian Wells]

On 29 May 2018 at 14:53, Jeremy Stanley  wrote:

> On 2018-05-29 15:25:01 -0500 (-0500), Jay S Bryant wrote:
> [...]
> > Maybe it would be different now that I am a Core/PTL but in the past I
> had
> > been warned to be careful as it could be misinterpreted if I was changing
> > other people's patches or that it could look like I was trying to pad my
> > numbers. (I am a nit-picker though I do my best not to be.
> [...]
>
> Most stats tracking goes by the Gerrit "Owner" metadata or the Git
> "Author" field, neither of which are modified in a typical new
> patchset workflow and so carry over from the original patchset #1
> (resetting Author requires creating a new commit from scratch or
> passing extra options to git to reset it, while changing the Owner
> needs a completely new Change-Id footer).
>

We know this, but other people don't, so the comment is wise.  Also,
arguably, if I badly fix someone else's patch, I'm making them look bad by
leaving them with the 'credit' for my bad work, so it's important to be
careful and tactful.  But the history is public record, at least.

-- 
Ian.
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jeremy Stanley]

On 2018-05-29 15:25:01 -0500 (-0500), Jay S Bryant wrote:
[...]
> Maybe it would be different now that I am a Core/PTL but in the past I had
> been warned to be careful as it could be misinterpreted if I was changing
> other people's patches or that it could look like I was trying to pad my
> numbers. (I am a nit-picker though I do my best not to be.
[...]

Most stats tracking goes by the Gerrit "Owner" metadata or the Git
"Author" field, neither of which are modified in a typical new
patchset workflow and so carry over from the original patchset #1
(resetting Author requires creating a new commit from scratch or
passing extra options to git to reset it, while changing the Owner
needs a completely new Change-Id footer).
-- 
Jeremy Stanley


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Doug Hellmann]

Excerpts from Slawomir Kaplonski's message of 2018-05-29 22:49:07 +0200:
> Hi,
> 
> > Wiadomość napisana przez Jay S Bryant  w dniu 
> > 29.05.2018, o godz. 22:25:
> > 
> > 
> > On 5/29/2018 3:19 PM, Doug Hellmann wrote:
> >> Excerpts from Jonathan Proulx's message of 2018-05-29 16:05:06 -0400:
> >>> On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
> >>> :> >> maybe we're all saying the same thing here?
> >>> :> > Yeah, I feel like we're all essentially in agreement that nits (of 
> >>> the
> >>> :> > English mistake of typo type) do need to get fixed, but sometimes
> >>> :> > (often?) putting the burden of fixing them on the original patch
> >>> :> > contributor is neither fair nor constructive.
> >>> :> I am ok with this statement if we are all in agreement that doing
> >>> :> follow-up patches is an acceptable practice.
> >>> :
> >>> :Has it ever not been?
> >>> :
> >>> :It seems like it has always come down to a bit of negotiation with
> >>> :the original author, hasn't it? And that won't change, except that
> >>> :we will be emphasizing to reviewers that we encourage them to be
> >>> :more active in seeking out that negotiation and then proposing
> >>> :patches?
> >>> 
> >>> Exactly, it's more codifying a default.
> >>> 
> >>> It's not been unacceptable but I think there's some understandable
> >>> reluctance to make changes to someone else's work, you don't want to
> >>> seem like your taking over or getting in the way.  At least that's
> >>> what's in my head when deciding should this be a comment or a patch.
> >>> 
> >>> I think this discussion suggests for certain class of "nits" patch is
> >>> preferred to comment.  If that is true making this explicit is a good
> >>> thing becuase let's face it my social skills are only marginally
> >>> better than my speeling :)
> >>> 
> >>> -Jon
> >>> 
> >> OK, that's all good. I'm just surprised to learn that throwing a
> >> follow-up patch on top of someone else's patch was ever seen as
> >> discouraged.
> >> 
> >> The spice must flow,
> >> Doug
> > 
> > Maybe it would be different now that I am a Core/PTL but in the past I had 
> > been warned to be careful as it could be misinterpreted if I was changing 
> > other people's patches or that it could look like I was trying to pad my 
> > numbers. (I am a nit-picker though I do my best not to be.
> 
> Exactly. I remember when I was doing my first patch (or one of first patches) 
> and someone pushed new PS with some very small nits fixed. I was a bit 
> confused because of that and I was thinking why he did it instead of me?
> Now it’s of course much more clear for me but for someone who is new 
> contributor I think that this might be confusing. Maybe such person should at 
> least remember to explain in comment why he pushed new PS and that’s not 
> „stealing” work of original author :)

I guess it never occurred to me that someone would do that without
also leaving a comment explaining the situation.

Doug

> 
> > 
> > I am happy if people understand I am just trying to keep the process moving 
> > and keep the read/flow of Cinder consistent.  :-)
> > 
> > Jay
> > 
> >> __
> >> OpenStack Development Mailing List (not for usage questions)
> >> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> > 
> > 
> > __
> > OpenStack Development Mailing List (not for usage questions)
> > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> — 
> Slawek Kaplonski
> Senior software engineer
> Red Hat
> 

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Slawomir Kaplonski]

Hi,

> Wiadomość napisana przez Jay S Bryant  w dniu 
> 29.05.2018, o godz. 22:25:
> 
> 
> On 5/29/2018 3:19 PM, Doug Hellmann wrote:
>> Excerpts from Jonathan Proulx's message of 2018-05-29 16:05:06 -0400:
>>> On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
>>> :> >> maybe we're all saying the same thing here?
>>> :> > Yeah, I feel like we're all essentially in agreement that nits (of the
>>> :> > English mistake of typo type) do need to get fixed, but sometimes
>>> :> > (often?) putting the burden of fixing them on the original patch
>>> :> > contributor is neither fair nor constructive.
>>> :> I am ok with this statement if we are all in agreement that doing
>>> :> follow-up patches is an acceptable practice.
>>> :
>>> :Has it ever not been?
>>> :
>>> :It seems like it has always come down to a bit of negotiation with
>>> :the original author, hasn't it? And that won't change, except that
>>> :we will be emphasizing to reviewers that we encourage them to be
>>> :more active in seeking out that negotiation and then proposing
>>> :patches?
>>> 
>>> Exactly, it's more codifying a default.
>>> 
>>> It's not been unacceptable but I think there's some understandable
>>> reluctance to make changes to someone else's work, you don't want to
>>> seem like your taking over or getting in the way.  At least that's
>>> what's in my head when deciding should this be a comment or a patch.
>>> 
>>> I think this discussion suggests for certain class of "nits" patch is
>>> preferred to comment.  If that is true making this explicit is a good
>>> thing becuase let's face it my social skills are only marginally
>>> better than my speeling :)
>>> 
>>> -Jon
>>> 
>> OK, that's all good. I'm just surprised to learn that throwing a
>> follow-up patch on top of someone else's patch was ever seen as
>> discouraged.
>> 
>> The spice must flow,
>> Doug
> 
> Maybe it would be different now that I am a Core/PTL but in the past I had 
> been warned to be careful as it could be misinterpreted if I was changing 
> other people's patches or that it could look like I was trying to pad my 
> numbers. (I am a nit-picker though I do my best not to be.

Exactly. I remember when I was doing my first patch (or one of first patches) 
and someone pushed new PS with some very small nits fixed. I was a bit confused 
because of that and I was thinking why he did it instead of me?
Now it’s of course much more clear for me but for someone who is new 
contributor I think that this might be confusing. Maybe such person should at 
least remember to explain in comment why he pushed new PS and that’s not 
„stealing” work of original author :)

> 
> I am happy if people understand I am just trying to keep the process moving 
> and keep the read/flow of Cinder consistent.  :-)
> 
> Jay
> 
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

— 
Slawek Kaplonski
Senior software engineer
Red Hat


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jay S Bryant]

On 5/29/2018 3:19 PM, Doug Hellmann wrote:

Excerpts from Jonathan Proulx's message of 2018-05-29 16:05:06 -0400:

On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
:> >> maybe we're all saying the same thing here?
:> > Yeah, I feel like we're all essentially in agreement that nits (of the
:> > English mistake of typo type) do need to get fixed, but sometimes
:> > (often?) putting the burden of fixing them on the original patch
:> > contributor is neither fair nor constructive.
:> I am ok with this statement if we are all in agreement that doing
:> follow-up patches is an acceptable practice.
:
:Has it ever not been?
:
:It seems like it has always come down to a bit of negotiation with
:the original author, hasn't it? And that won't change, except that
:we will be emphasizing to reviewers that we encourage them to be
:more active in seeking out that negotiation and then proposing
:patches?

Exactly, it's more codifying a default.

It's not been unacceptable but I think there's some understandable
reluctance to make changes to someone else's work, you don't want to
seem like your taking over or getting in the way.  At least that's
what's in my head when deciding should this be a comment or a patch.

I think this discussion suggests for certain class of "nits" patch is
preferred to comment.  If that is true making this explicit is a good
thing becuase let's face it my social skills are only marginally
better than my speeling :)

-Jon


OK, that's all good. I'm just surprised to learn that throwing a
follow-up patch on top of someone else's patch was ever seen as
discouraged.

The spice must flow,
Doug


Maybe it would be different now that I am a Core/PTL but in the past I 
had been warned to be careful as it could be misinterpreted if I was 
changing other people's patches or that it could look like I was trying 
to pad my numbers. (I am a nit-picker though I do my best not to be.


I am happy if people understand I am just trying to keep the process 
moving and keep the read/flow of Cinder consistent.  :-)


Jay


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Doug Hellmann]

Excerpts from Jonathan Proulx's message of 2018-05-29 16:05:06 -0400:
> On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
> :> >> maybe we're all saying the same thing here?
> :> > Yeah, I feel like we're all essentially in agreement that nits (of the
> :> > English mistake of typo type) do need to get fixed, but sometimes
> :> > (often?) putting the burden of fixing them on the original patch
> :> > contributor is neither fair nor constructive.
> :> I am ok with this statement if we are all in agreement that doing 
> :> follow-up patches is an acceptable practice.
> :
> :Has it ever not been?
> :
> :It seems like it has always come down to a bit of negotiation with
> :the original author, hasn't it? And that won't change, except that
> :we will be emphasizing to reviewers that we encourage them to be
> :more active in seeking out that negotiation and then proposing
> :patches?
> 
> Exactly, it's more codifying a default.
> 
> It's not been unacceptable but I think there's some understandable
> reluctance to make changes to someone else's work, you don't want to
> seem like your taking over or getting in the way.  At least that's
> what's in my head when deciding should this be a comment or a patch.
> 
> I think this discussion suggests for certain class of "nits" patch is
> preferred to comment.  If that is true making this explicit is a good
> thing becuase let's face it my social skills are only marginally
> better than my speeling :)
> 
> -Jon
> 

OK, that's all good. I'm just surprised to learn that throwing a
follow-up patch on top of someone else's patch was ever seen as
discouraged.

The spice must flow,
Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jonathan Proulx]

On Tue, May 29, 2018 at 03:53:41PM -0400, Doug Hellmann wrote:
:> >> maybe we're all saying the same thing here?
:> > Yeah, I feel like we're all essentially in agreement that nits (of the
:> > English mistake of typo type) do need to get fixed, but sometimes
:> > (often?) putting the burden of fixing them on the original patch
:> > contributor is neither fair nor constructive.
:> I am ok with this statement if we are all in agreement that doing 
:> follow-up patches is an acceptable practice.
:
:Has it ever not been?
:
:It seems like it has always come down to a bit of negotiation with
:the original author, hasn't it? And that won't change, except that
:we will be emphasizing to reviewers that we encourage them to be
:more active in seeking out that negotiation and then proposing
:patches?

Exactly, it's more codifying a default.

It's not been unacceptable but I think there's some understandable
reluctance to make changes to someone else's work, you don't want to
seem like your taking over or getting in the way.  At least that's
what's in my head when deciding should this be a comment or a patch.

I think this discussion suggests for certain class of "nits" patch is
preferred to comment.  If that is true making this explicit is a good
thing becuase let's face it my social skills are only marginally
better than my speeling :)

-Jon

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Doug Hellmann]

Excerpts from Julia Kreger's message of 2018-05-29 15:41:55 -0400:
> On Tue, May 29, 2018 at 3:16 PM, Jay S Bryant  wrote:
> >
> >
> > On 5/29/2018 2:06 PM, Artom Lifshitz wrote:
> >> Yeah, I feel like we're all essentially in agreement that nits (of the
> >> English mistake of typo type) do need to get fixed, but sometimes
> >> (often?) putting the burden of fixing them on the original patch
> >> contributor is neither fair nor constructive.
> >
> > I am ok with this statement if we are all in agreement that doing follow-up
> > patches is an acceptable practice.
> >
> It does feel like there is some general agreement. \o/
> 
> Putting my Ironic hat on, we've been trying to stress that follow-up
> patches are totally acceptable and encouraged. Follow-up patches seem
> to land faster in the grand scheme of things and allow series of
> patches to move forward in the mean time which is important when a
> feature may be spread across 10+ patches
> 
> As for editing just prior to approving, we have learned there can be
> absolutely no delay between that edit being made and the patch
> approved to land. In essence patches would begin to look like only a
> single core reviewer had approved the change they just edited even if
> the prior revision had a second core approving the prior revision.. In
> my experience, the async nature of waiting for a second core to
> sign-off on your edits incurs additional time for nitpicks to occur
> and a patch to be blocked.
> 
> Sadly putting the burden on the person approving changes to land is a
> bit much as well. I think anyone should be free to propose a follow-up
> to any patch, at least that is my opinion and why I wrote the
> principles change as I did.
> 

+1 to that last bit, for sure.

In several conversations about this last week we discussed the
impression that we don't often see +1 votes with useful comments.
A +1 with a follow-up to fix minor issues seems like something we
ought to encourage.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Doug Hellmann]

Excerpts from Jay S Bryant's message of 2018-05-29 14:16:33 -0500:
> 
> On 5/29/2018 2:06 PM, Artom Lifshitz wrote:
> >> On Tue, May 29, 2018 at 10:52:04AM -0400, Mohammed Naser wrote:
> >>
> >> :On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  
> >> wrote:
> >> :>  One idea would be that, once the meat of the patch
> >> :> has passed multiple rounds of reviews and looks good, and what remains
> >> :> is only nits, the reviewer themselves take on the responsibility of
> >> :> pushing a new patch that fixes the nits that they found.
> >>
> >> Doesn't the above suggestion sufficiently address the concern below?
> >>
> >> :I'd just like to point out that what you perceive as a 'finished
> >> :product that looks unprofessional' might be already hard enough for a
> >> :contributor to achieve.  We have a lot of new contributors coming from
> >> :all over the world and it is very discouraging for them to have their
> >> :technical knowledge and work be categorized as 'unprofessional'
> >> :because of the language barrier.
> >> :
> >> :git-nit and a few minutes of your time will go a long way, IMHO.
> >>
> >> As very intermittent contributor and native english speaker with
> >> relatively poor spelling and typing I'd be much happier with a
> >> reviewer pushing a patch that fixes nits rather than having a ton of
> >> inline comments that point them out.
> >>
> >> maybe we're all saying the same thing here?
> > Yeah, I feel like we're all essentially in agreement that nits (of the
> > English mistake of typo type) do need to get fixed, but sometimes
> > (often?) putting the burden of fixing them on the original patch
> > contributor is neither fair nor constructive.
> I am ok with this statement if we are all in agreement that doing 
> follow-up patches is an acceptable practice.

Has it ever not been?

It seems like it has always come down to a bit of negotiation with
the original author, hasn't it? And that won't change, except that
we will be emphasizing to reviewers that we encourage them to be
more active in seeking out that negotiation and then proposing
patches?

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Davanum Srinivas]

Agree with Ian here.

Also another problem that comes up is: "Why are you touching *MY*
review?" (probably coming from the view where stats - and stackalytics
leaderboard position is important). So i guess we ask permission
before editing (or) file a follow up later (or) just tell folks that
this is ok to do!!

Hoping engaging with them will solve yet another issue is someone
going around filing the same change in a dozen projects (repeatedly!),
but that may be wishful thinking.

-- Dims

On Tue, May 29, 2018 at 12:17 PM, Ian Wells  wrote:
> If your nitpick is a spelling mistake or the need for a comment where you've
> pretty much typed the text of the comment in the review comment itself, then
> I have personally found it easiest to use the Gerrit online editor to
> actually update the patch yourself.  There's nothing magical about the
> original submitter, and no point in wasting your time and theirs to get them
> to make the change.  That said, please be a grown up; if you're changing
> code or messing up formatting enough for PEP8 to be a concern, it's your
> responsibility, not the original submitter's, to fix it.  Also, do all your
> fixes in one commit if you don't want to make Zuul cry.
> --
> Ian.
>
>
> On 29 May 2018 at 09:00, Neil Jerram  wrote:
>>
>> From my point of view as someone who is still just an occasional
>> contributor (in all OpenStack projects other than my own team's networking
>> driver), and so I think still sensitive to the concerns being raised here:
>>
>> - Nits are not actually a problem, at all, if they are uncontroversial and
>> quick to deal with.  For example, if it's a point of English, and most
>> English speakers would agree that a correction is better, it's quick and no
>> problem for me to make that correction.
>>
>> - What is much more of a problem is:
>>
>>   - Anything that is more a matter of opinion.  If a markup is just the
>> reviewer's personal opinion, and they can't say anything to explain more
>> objectively why their suggestion is better, it would be wiser to defer to
>> the contributor's initial choice.
>>
>>   - Questioning something unconstructively or out of proportion to the
>> change being made.  This is a tricky one to pin down, but sometimes I've had
>> comments that raise some random left-field question that isn't really
>> related to the change being made, or where the reviewer could have done a
>> couple minutes research themselves and then either made a more precise
>> comment, or not made their comment at all.
>>
>>   - Asking - implicitly or explicitly - the contributor to add more
>> cleanups to their change.  If someone usefully fixes a problem, and their
>> fix does not of itself impair the quality or maintainability of the
>> surrounding code, they should not be asked to extend their fix so as to fix
>> further problems that a more regular developer may be aware of in that area,
>> or to advance a refactoring / cleanup that another developer has in mind.
>> (At least, not as part of that initial change.)
>>
>> (Obviously the common thread of those problem points is taking up more
>> time; psychologically I think one of the things that can turn a contributor
>> away is the feeling that they've contributed a clearly useful thing, yet the
>> community is stalling over accepting it for reasons that do not appear
>> clearcut.)
>>
>> Hoping this is vaguely helpful...
>>  Neil
>>
>>
>> On Tue, May 29, 2018 at 4:35 PM Amy Marrich  wrote:
>>>
>>> If I have a nit that doesn't affect things, I'll make a note of it and
>>> say if you do another patch I'd really like it fixed but also give the patch
>>> a vote. What I'll also do sometimes if I know the user or they are online
>>> I'll offer to fix things for them, that way they can see what I've done,
>>> I've sped things along and I haven't caused a simple change to take a long
>>> amount of time and reviews.
>>>
>>> I think this is a great addition!
>>>
>>> Thanks,
>>>
>>> Amy (spotz)
>>>
>>> On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
>>>  wrote:

 During the Forum, the topic of review culture came up in session after
 session. During these discussions, the subject of our use of nitpicks
 were often raised as a point of contention and frustration, especially
 by community members that have left the community and that were
 attempting to re-engage the community. Contributors raised the point
 of review feedback requiring for extremely precise English, or
 compliance to a particular core reviewer's style preferences, which
 may not be the same as another core reviewer.

 These things are not just frustrating, but also very inhibiting for
 part time contributors such as students who may also be time limited.
 Or an operator who noticed something that was clearly a bug and that
 put forth a very minor fix and doesn't have the time to revise it over
 and over.

 While nitpicks do help guide and teach, the consensus seemed to be

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Julia Kreger]

On Tue, May 29, 2018 at 3:16 PM, Jay S Bryant  wrote:
>
>
> On 5/29/2018 2:06 PM, Artom Lifshitz wrote:
>> Yeah, I feel like we're all essentially in agreement that nits (of the
>> English mistake of typo type) do need to get fixed, but sometimes
>> (often?) putting the burden of fixing them on the original patch
>> contributor is neither fair nor constructive.
>
> I am ok with this statement if we are all in agreement that doing follow-up
> patches is an acceptable practice.
>
It does feel like there is some general agreement. \o/

Putting my Ironic hat on, we've been trying to stress that follow-up
patches are totally acceptable and encouraged. Follow-up patches seem
to land faster in the grand scheme of things and allow series of
patches to move forward in the mean time which is important when a
feature may be spread across 10+ patches

As for editing just prior to approving, we have learned there can be
absolutely no delay between that edit being made and the patch
approved to land. In essence patches would begin to look like only a
single core reviewer had approved the change they just edited even if
the prior revision had a second core approving the prior revision.. In
my experience, the async nature of waiting for a second core to
sign-off on your edits incurs additional time for nitpicks to occur
and a patch to be blocked.

Sadly putting the burden on the person approving changes to land is a
bit much as well. I think anyone should be free to propose a follow-up
to any patch, at least that is my opinion and why I wrote the
principles change as I did.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Ian Wells]

If your nitpick is a spelling mistake or the need for a comment where
you've pretty much typed the text of the comment in the review comment
itself, then I have personally found it easiest to use the Gerrit online
editor to actually update the patch yourself.  There's nothing magical
about the original submitter, and no point in wasting your time and theirs
to get them to make the change.  That said, please be a grown up; if you're
changing code or messing up formatting enough for PEP8 to be a concern,
it's your responsibility, not the original submitter's, to fix it.  Also,
do all your fixes in one commit if you don't want to make Zuul cry.
-- 
Ian.


On 29 May 2018 at 09:00, Neil Jerram  wrote:

> From my point of view as someone who is still just an occasional
> contributor (in all OpenStack projects other than my own team's networking
> driver), and so I think still sensitive to the concerns being raised here:
>
> - Nits are not actually a problem, at all, if they are uncontroversial and
> quick to deal with.  For example, if it's a point of English, and most
> English speakers would agree that a correction is better, it's quick and no
> problem for me to make that correction.
>
> - What is much more of a problem is:
>
>   - Anything that is more a matter of opinion.  If a markup is just the
> reviewer's personal opinion, and they can't say anything to explain more
> objectively why their suggestion is better, it would be wiser to defer to
> the contributor's initial choice.
>
>   - Questioning something unconstructively or out of proportion to the
> change being made.  This is a tricky one to pin down, but sometimes I've
> had comments that raise some random left-field question that isn't really
> related to the change being made, or where the reviewer could have done a
> couple minutes research themselves and then either made a more precise
> comment, or not made their comment at all.
>
>   - Asking - implicitly or explicitly - the contributor to add more
> cleanups to their change.  If someone usefully fixes a problem, and their
> fix does not of itself impair the quality or maintainability of the
> surrounding code, they should not be asked to extend their fix so as to fix
> further problems that a more regular developer may be aware of in that
> area, or to advance a refactoring / cleanup that another developer has in
> mind.  (At least, not as part of that initial change.)
>
> (Obviously the common thread of those problem points is taking up more
> time; psychologically I think one of the things that can turn a contributor
> away is the feeling that they've contributed a clearly useful thing, yet
> the community is stalling over accepting it for reasons that do not appear
> clearcut.)
>
> Hoping this is vaguely helpful...
>  Neil
>
>
> On Tue, May 29, 2018 at 4:35 PM Amy Marrich  wrote:
>
>> If I have a nit that doesn't affect things, I'll make a note of it and
>> say if you do another patch I'd really like it fixed but also give the
>> patch a vote. What I'll also do sometimes if I know the user or they are
>> online I'll offer to fix things for them, that way they can see what I've
>> done, I've sped things along and I haven't caused a simple change to take a
>> long amount of time and reviews.
>>
>> I think this is a great addition!
>>
>> Thanks,
>>
>> Amy (spotz)
>>
>> On Tue, May 29, 2018 at 6:55 AM, Julia Kreger <
>> juliaashleykre...@gmail.com> wrote:
>>
>>> During the Forum, the topic of review culture came up in session after
>>> session. During these discussions, the subject of our use of nitpicks
>>> were often raised as a point of contention and frustration, especially
>>> by community members that have left the community and that were
>>> attempting to re-engage the community. Contributors raised the point
>>> of review feedback requiring for extremely precise English, or
>>> compliance to a particular core reviewer's style preferences, which
>>> may not be the same as another core reviewer.
>>>
>>> These things are not just frustrating, but also very inhibiting for
>>> part time contributors such as students who may also be time limited.
>>> Or an operator who noticed something that was clearly a bug and that
>>> put forth a very minor fix and doesn't have the time to revise it over
>>> and over.
>>>
>>> While nitpicks do help guide and teach, the consensus seemed to be
>>> that we do need to shift the culture a little bit. As such, I've
>>> proposed a change to our principles[1] in governance that attempts to
>>> capture the essence and spirit of the nitpicking topic as a first
>>> step.
>>>
>>> -Julia
>>> -
>>> [1]: https://review.openstack.org/570940
>>>
>>> 
>>> __
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:
>>> unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>
>> 

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jay S Bryant]

On 5/29/2018 2:06 PM, Artom Lifshitz wrote:

On Tue, May 29, 2018 at 10:52:04AM -0400, Mohammed Naser wrote:

:On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  wrote:
:>  One idea would be that, once the meat of the patch
:> has passed multiple rounds of reviews and looks good, and what remains
:> is only nits, the reviewer themselves take on the responsibility of
:> pushing a new patch that fixes the nits that they found.

Doesn't the above suggestion sufficiently address the concern below?

:I'd just like to point out that what you perceive as a 'finished
:product that looks unprofessional' might be already hard enough for a
:contributor to achieve.  We have a lot of new contributors coming from
:all over the world and it is very discouraging for them to have their
:technical knowledge and work be categorized as 'unprofessional'
:because of the language barrier.
:
:git-nit and a few minutes of your time will go a long way, IMHO.

As very intermittent contributor and native english speaker with
relatively poor spelling and typing I'd be much happier with a
reviewer pushing a patch that fixes nits rather than having a ton of
inline comments that point them out.

maybe we're all saying the same thing here?

Yeah, I feel like we're all essentially in agreement that nits (of the
English mistake of typo type) do need to get fixed, but sometimes
(often?) putting the burden of fixing them on the original patch
contributor is neither fair nor constructive.
I am ok with this statement if we are all in agreement that doing 
follow-up patches is an acceptable practice.



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Artom Lifshitz]

> On Tue, May 29, 2018 at 10:52:04AM -0400, Mohammed Naser wrote:
>
> :On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  wrote:
> :>  One idea would be that, once the meat of the patch
> :> has passed multiple rounds of reviews and looks good, and what remains
> :> is only nits, the reviewer themselves take on the responsibility of
> :> pushing a new patch that fixes the nits that they found.
>
> Doesn't the above suggestion sufficiently address the concern below?
>
> :I'd just like to point out that what you perceive as a 'finished
> :product that looks unprofessional' might be already hard enough for a
> :contributor to achieve.  We have a lot of new contributors coming from
> :all over the world and it is very discouraging for them to have their
> :technical knowledge and work be categorized as 'unprofessional'
> :because of the language barrier.
> :
> :git-nit and a few minutes of your time will go a long way, IMHO.
>
> As very intermittent contributor and native english speaker with
> relatively poor spelling and typing I'd be much happier with a
> reviewer pushing a patch that fixes nits rather than having a ton of
> inline comments that point them out.
>
> maybe we're all saying the same thing here?

Yeah, I feel like we're all essentially in agreement that nits (of the
English mistake of typo type) do need to get fixed, but sometimes
(often?) putting the burden of fixing them on the original patch
contributor is neither fair nor constructive.

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jay S Bryant]

Julia,

Thank you for starting this discussion.


On 5/29/2018 9:43 AM, Artom Lifshitz wrote:

I dunno, there's a fine line to be drawn between getting a finished
product that looks unprofessional (because of typos, English mistakes,
etc), and nitpicking to the point of smothering and being
counter-productive. One idea would be that, once the meat of the patch
has passed multiple rounds of reviews and looks good, and what remains
is only nits, the reviewer themselves take on the responsibility of
pushing a new patch that fixes the nits that they found.
In the past this is something that I have wanted to do but have received 
mixed feedback on its level of appropriateness.  I am happy to push 
follow-up patches to address nit-picks rather than hold up a patch.  We, 
however, will need to communicate to the community that this is now an 
acceptable practice.



On Tue, May 29, 2018 at 9:55 AM, Julia Kreger
 wrote:

During the Forum, the topic of review culture came up in session after
session. During these discussions, the subject of our use of nitpicks
were often raised as a point of contention and frustration, especially
by community members that have left the community and that were
attempting to re-engage the community. Contributors raised the point
of review feedback requiring for extremely precise English, or
compliance to a particular core reviewer's style preferences, which
may not be the same as another core reviewer.

These things are not just frustrating, but also very inhibiting for
part time contributors such as students who may also be time limited.
Or an operator who noticed something that was clearly a bug and that
put forth a very minor fix and doesn't have the time to revise it over
and over.

While nitpicks do help guide and teach, the consensus seemed to be
that we do need to shift the culture a little bit. As such, I've
proposed a change to our principles[1] in governance that attempts to
capture the essence and spirit of the nitpicking topic as a first
step.

-Julia
-
[1]: https://review.openstack.org/570940

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev






__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Davanum Srinivas]

Thanks for driving this Julia. +1. It's time to do this.

-- Dims

On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
 wrote:
> During the Forum, the topic of review culture came up in session after
> session. During these discussions, the subject of our use of nitpicks
> were often raised as a point of contention and frustration, especially
> by community members that have left the community and that were
> attempting to re-engage the community. Contributors raised the point
> of review feedback requiring for extremely precise English, or
> compliance to a particular core reviewer's style preferences, which
> may not be the same as another core reviewer.
>
> These things are not just frustrating, but also very inhibiting for
> part time contributors such as students who may also be time limited.
> Or an operator who noticed something that was clearly a bug and that
> put forth a very minor fix and doesn't have the time to revise it over
> and over.
>
> While nitpicks do help guide and teach, the consensus seemed to be
> that we do need to shift the culture a little bit. As such, I've
> proposed a change to our principles[1] in governance that attempts to
> capture the essence and spirit of the nitpicking topic as a first
> step.
>
> -Julia
> -
> [1]: https://review.openstack.org/570940
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



-- 
Davanum Srinivas :: https://twitter.com/dims

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Joshua Harlow]

Jonathan D. Proulx wrote:

On Tue, May 29, 2018 at 10:52:04AM -0400, Mohammed Naser wrote:

:On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  wrote:
:>   One idea would be that, once the meat of the patch
:>  has passed multiple rounds of reviews and looks good, and what remains
:>  is only nits, the reviewer themselves take on the responsibility of
:>  pushing a new patch that fixes the nits that they found.

Doesn't the above suggestion sufficiently address the concern below?

:I'd just like to point out that what you perceive as a 'finished
:product that looks unprofessional' might be already hard enough for a
:contributor to achieve.  We have a lot of new contributors coming from
:all over the world and it is very discouraging for them to have their
:technical knowledge and work be categorized as 'unprofessional'
:because of the language barrier.
:
:git-nit and a few minutes of your time will go a long way, IMHO.

As very intermittent contributor and native english speaker with
relatively poor spelling and typing I'd be much happier with a
reviewer pushing a patch that fixes nits rather than having a ton of
inline comments that point them out.

maybe we're all saying the same thing here?


https://sep.yimg.com/ay/computergear/i-write-code-computer-t-shirt-13.gif

I am the same ;)



-JOn

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Mathieu Gagné]

Hi Julia,

Thanks for the follow up on this topic.

On Tue, May 29, 2018 at 6:55 AM, Julia Kreger
 wrote:
>
> These things are not just frustrating, but also very inhibiting for
> part time contributors such as students who may also be time limited.
> Or an operator who noticed something that was clearly a bug and that
> put forth a very minor fix and doesn't have the time to revise it over
> and over.
>

What I found frustrating is receiving *only* nitpicks, addressing them
to only receive more nitpicks (sometimes from the same reviewer) with
no substantial review on the change itself afterward.
I wouldn't mind addressing nitpicks if more substantial reviews were
made in a timely fashion.

--
Mathieu

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Jonathan D. Proulx]

On Tue, May 29, 2018 at 10:52:04AM -0400, Mohammed Naser wrote:

:On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  wrote:
:>  One idea would be that, once the meat of the patch
:> has passed multiple rounds of reviews and looks good, and what remains
:> is only nits, the reviewer themselves take on the responsibility of
:> pushing a new patch that fixes the nits that they found.

Doesn't the above suggestion sufficiently address the concern below?

:I'd just like to point out that what you perceive as a 'finished
:product that looks unprofessional' might be already hard enough for a
:contributor to achieve.  We have a lot of new contributors coming from
:all over the world and it is very discouraging for them to have their
:technical knowledge and work be categorized as 'unprofessional'
:because of the language barrier.
:
:git-nit and a few minutes of your time will go a long way, IMHO.

As very intermittent contributor and native english speaker with
relatively poor spelling and typing I'd be much happier with a
reviewer pushing a patch that fixes nits rather than having a ton of
inline comments that point them out.

maybe we're all saying the same thing here?

-JOn

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Neil Jerram]

>From my point of view as someone who is still just an occasional
contributor (in all OpenStack projects other than my own team's networking
driver), and so I think still sensitive to the concerns being raised here:

- Nits are not actually a problem, at all, if they are uncontroversial and
quick to deal with.  For example, if it's a point of English, and most
English speakers would agree that a correction is better, it's quick and no
problem for me to make that correction.

- What is much more of a problem is:

  - Anything that is more a matter of opinion.  If a markup is just the
reviewer's personal opinion, and they can't say anything to explain more
objectively why their suggestion is better, it would be wiser to defer to
the contributor's initial choice.

  - Questioning something unconstructively or out of proportion to the
change being made.  This is a tricky one to pin down, but sometimes I've
had comments that raise some random left-field question that isn't really
related to the change being made, or where the reviewer could have done a
couple minutes research themselves and then either made a more precise
comment, or not made their comment at all.

  - Asking - implicitly or explicitly - the contributor to add more
cleanups to their change.  If someone usefully fixes a problem, and their
fix does not of itself impair the quality or maintainability of the
surrounding code, they should not be asked to extend their fix so as to fix
further problems that a more regular developer may be aware of in that
area, or to advance a refactoring / cleanup that another developer has in
mind.  (At least, not as part of that initial change.)

(Obviously the common thread of those problem points is taking up more
time; psychologically I think one of the things that can turn a contributor
away is the feeling that they've contributed a clearly useful thing, yet
the community is stalling over accepting it for reasons that do not appear
clearcut.)

Hoping this is vaguely helpful...
 Neil


On Tue, May 29, 2018 at 4:35 PM Amy Marrich  wrote:

> If I have a nit that doesn't affect things, I'll make a note of it and say
> if you do another patch I'd really like it fixed but also give the patch a
> vote. What I'll also do sometimes if I know the user or they are online
> I'll offer to fix things for them, that way they can see what I've done,
> I've sped things along and I haven't caused a simple change to take a long
> amount of time and reviews.
>
> I think this is a great addition!
>
> Thanks,
>
> Amy (spotz)
>
> On Tue, May 29, 2018 at 6:55 AM, Julia Kreger  > wrote:
>
>> During the Forum, the topic of review culture came up in session after
>> session. During these discussions, the subject of our use of nitpicks
>> were often raised as a point of contention and frustration, especially
>> by community members that have left the community and that were
>> attempting to re-engage the community. Contributors raised the point
>> of review feedback requiring for extremely precise English, or
>> compliance to a particular core reviewer's style preferences, which
>> may not be the same as another core reviewer.
>>
>> These things are not just frustrating, but also very inhibiting for
>> part time contributors such as students who may also be time limited.
>> Or an operator who noticed something that was clearly a bug and that
>> put forth a very minor fix and doesn't have the time to revise it over
>> and over.
>>
>> While nitpicks do help guide and teach, the consensus seemed to be
>> that we do need to shift the culture a little bit. As such, I've
>> proposed a change to our principles[1] in governance that attempts to
>> capture the essence and spirit of the nitpicking topic as a first
>> step.
>>
>> -Julia
>> -
>> [1]: https://review.openstack.org/570940
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Amy Marrich]

If I have a nit that doesn't affect things, I'll make a note of it and say
if you do another patch I'd really like it fixed but also give the patch a
vote. What I'll also do sometimes if I know the user or they are online
I'll offer to fix things for them, that way they can see what I've done,
I've sped things along and I haven't caused a simple change to take a long
amount of time and reviews.

I think this is a great addition!

Thanks,

Amy (spotz)

On Tue, May 29, 2018 at 6:55 AM, Julia Kreger 
wrote:

> During the Forum, the topic of review culture came up in session after
> session. During these discussions, the subject of our use of nitpicks
> were often raised as a point of contention and frustration, especially
> by community members that have left the community and that were
> attempting to re-engage the community. Contributors raised the point
> of review feedback requiring for extremely precise English, or
> compliance to a particular core reviewer's style preferences, which
> may not be the same as another core reviewer.
>
> These things are not just frustrating, but also very inhibiting for
> part time contributors such as students who may also be time limited.
> Or an operator who noticed something that was clearly a bug and that
> put forth a very minor fix and doesn't have the time to revise it over
> and over.
>
> While nitpicks do help guide and teach, the consensus seemed to be
> that we do need to shift the culture a little bit. As such, I've
> proposed a change to our principles[1] in governance that attempts to
> capture the essence and spirit of the nitpicking topic as a first
> step.
>
> -Julia
> -
> [1]: https://review.openstack.org/570940
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Mohammed Naser]

On Tue, May 29, 2018 at 10:43 AM, Artom Lifshitz  wrote:
> I dunno, there's a fine line to be drawn between getting a finished
> product that looks unprofessional (because of typos, English mistakes,
> etc), and nitpicking to the point of smothering and being
> counter-productive. One idea would be that, once the meat of the patch
> has passed multiple rounds of reviews and looks good, and what remains
> is only nits, the reviewer themselves take on the responsibility of
> pushing a new patch that fixes the nits that they found.

I'd just like to point out that what you perceive as a 'finished
product that looks unprofessional' might be already hard enough for a
contributor to achieve.  We have a lot of new contributors coming from
all over the world and it is very discouraging for them to have their
technical knowledge and work be categorized as 'unprofessional'
because of the language barrier.

git-nit and a few minutes of your time will go a long way, IMHO.

> On Tue, May 29, 2018 at 9:55 AM, Julia Kreger
>  wrote:
>> During the Forum, the topic of review culture came up in session after
>> session. During these discussions, the subject of our use of nitpicks
>> were often raised as a point of contention and frustration, especially
>> by community members that have left the community and that were
>> attempting to re-engage the community. Contributors raised the point
>> of review feedback requiring for extremely precise English, or
>> compliance to a particular core reviewer's style preferences, which
>> may not be the same as another core reviewer.
>>
>> These things are not just frustrating, but also very inhibiting for
>> part time contributors such as students who may also be time limited.
>> Or an operator who noticed something that was clearly a bug and that
>> put forth a very minor fix and doesn't have the time to revise it over
>> and over.
>>
>> While nitpicks do help guide and teach, the consensus seemed to be
>> that we do need to shift the culture a little bit. As such, I've
>> proposed a change to our principles[1] in governance that attempts to
>> capture the essence and spirit of the nitpicking topic as a first
>> step.
>>
>> -Julia
>> -
>> [1]: https://review.openstack.org/570940
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
> --
> --
> Artom Lifshitz
> Software Engineer, OpenStack Compute DFG
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [tc][all] A culture change (nitpicking)

[Artom Lifshitz]

I dunno, there's a fine line to be drawn between getting a finished
product that looks unprofessional (because of typos, English mistakes,
etc), and nitpicking to the point of smothering and being
counter-productive. One idea would be that, once the meat of the patch
has passed multiple rounds of reviews and looks good, and what remains
is only nits, the reviewer themselves take on the responsibility of
pushing a new patch that fixes the nits that they found.

On Tue, May 29, 2018 at 9:55 AM, Julia Kreger
 wrote:
> During the Forum, the topic of review culture came up in session after
> session. During these discussions, the subject of our use of nitpicks
> were often raised as a point of contention and frustration, especially
> by community members that have left the community and that were
> attempting to re-engage the community. Contributors raised the point
> of review feedback requiring for extremely precise English, or
> compliance to a particular core reviewer's style preferences, which
> may not be the same as another core reviewer.
>
> These things are not just frustrating, but also very inhibiting for
> part time contributors such as students who may also be time limited.
> Or an operator who noticed something that was clearly a bug and that
> put forth a very minor fix and doesn't have the time to revise it over
> and over.
>
> While nitpicks do help guide and teach, the consensus seemed to be
> that we do need to shift the culture a little bit. As such, I've
> proposed a change to our principles[1] in governance that attempts to
> capture the essence and spirit of the nitpicking topic as a first
> step.
>
> -Julia
> -
> [1]: https://review.openstack.org/570940
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



-- 
--
Artom Lifshitz
Software Engineer, OpenStack Compute DFG

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev