Rant regarding code review issues

Rant regarding code review issues

[Wilfred Smith]

I believe this is a justified rant. This is not intended to offend, but to assert that the process is broken.

I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.

When I go tooling down US-101 at 90 mph in the carpool lane, texting my illegal alien girlfriend as I’m driving to the convenience store, in a stolen red convertible, with a bottle of fine scotch in one hand and an unregistered AK in the other, at least I know which rules I’m violating or likely to be violating when I hear helicopters overhead. 

But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and (b) I may legitimately disagree with (e.g. the insanity that anyone holds to an 80-column rule instead of 132 in a day of 4K monitors and 75-character class names inside nested namespaces that are just as long, not counting fully decorated traits). Oh…right…just use “auto”.

The process should be somewhat predictable, preferably programmatic.

Mind you, I have no problem with complying with the maintainer’s rules, but I’d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?

It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.

Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.

Now forgive me for my rant. I gotta pick up some M&M’s for my girlfriend before 5th period or she’ll be very cross with me.

Re: Rant regarding code review issues

[krtaylor]

On 9/13/19 1:52 PM, Wilfred Smith wrote:
> I believe this is a justified rant. This is not intended to offend, but to assert that the process is broken.

Welcome to open source! :) Seriously, thanks for bringing this up. The 
project belongs to the community. If you don't like something, feel free 
to jump in IRC, come to a working group meeting, or do this (thanks 
again) and suggest a fix.

> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
> 
> When I go tooling down US-101 at 90 mph in the carpool lane, texting my illegal alien girlfriend as I’m driving to the convenience store, in a stolen red convertible, with a bottle of fine scotch in one hand and an unregistered AK in the other, at least I know which rules I’m violating or likely to be violating when I hear helicopters overhead.
> 
> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and (b) I may legitimately disagree with (e.g. the insanity that anyone holds to an 80-column rule instead of 132 in a day of 4K monitors and 75-character class names inside nested namespaces that are just as long, not counting fully decorated traits). Oh…right…just use “auto”.
> 
> The process should be somewhat predictable, preferably programmatic.

As I said in the last sentence above, please propose fixes or just get 
it done and see who complains. I am here-by empowering you! Help us fix it!

> Mind you, I have no problem with complying with the maintainer’s rules, but I’d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?
> 
> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
> 
> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
> 
> Now forgive me for my rant. I gotta pick up some M&M’s for my girlfriend before 5th period or she’ll be very cross with me.

Funny stuff! Jump in IRC and start a conversation with one of the 
maintainers, folks around here are usually very friendly and helpful.

Kurt Taylor (krtaylor)

Re: Rant regarding code review issues

[Ed Tanous]

On 9/13/19 11:52 AM, Wilfred Smith wrote:
> 
> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
> 
https://github.com/openbmc/docs

> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
holds to an 80-column rule instead of 132 in a day of 4K monitors and
75-character class names inside nested namespaces that are just as long,
not counting fully decorated traits). Oh…right…just use “auto”.

From the coding standard here:
https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md

"Line length should be limited to 80 characters."

You are welcome to disagree with that statement, but the recommended way
to go about that would be to push a review to change to that rule,
invoking a 132 character line length, and start a discussion about the
above.  After that, follow up with changes to the clang-format, and
commits to each repository to change the codebase to obey the new rule.
I'm willing to bet if you did that, many people would agree with you,
and we'd have a very productive discussion about it.

It should be noted, for most code, all of this formatting is automatic
via clang-format, which is documented on the next line in the file I linked.

With all of the above, I'm struggling to see how you're wanting to
improve here.  Do you think the documentation could be cleaned up?  Is
the documentation too long to read?  Should we organize it in a
different way so that the information above is easier to find?

> 
> The process should be somewhat predictable, preferably programmatic.

Agreed, but throughout this email, I don't see any actionable suggestion
on how this could be improved.  Could you focus a little more on what
you think we could change to make this situation better?

> 
> Mind you, I have no problem with complying with the maintainer’s rules, but I’d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?

Assuming this is the review you're talking about:
https://gerrit.openbmc-project.xyz/c/openbmc/meta-facebook/+/25145

It was 7 lines of bitbake config, and it followed some project practices
that are deprecated and trying to be removed.

Interestingly enough, I'm not able to find where we document the 50/72
rule for commit messages, although I know some editors will enforce it
when you do your edits.  I would've expected it to be here:
https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
Maybe that's an improvement to the docs you could propose?

> 
> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.

Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
character line lengths) the CI could give faster feedback for obvious
things.  Is that something you'd be willing to look into automating in
the CI?  I'm sure a lot of people would appreciate you doing so.
There's a lot of low hanging fruit here that could be added to CI to
make the responses seem less hostile.

> 
> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.

At the end of the day, the commit message is what most people are
looking at once the commit is merged, and you put a single line that
wasn't very descriptive.  The maintainer pointed out that you already
had the level of detail needed, you just failed to put it in the commit
message.  Certainly, writing this email took longer than just updating
the commit message with more detail?

Re: Rant regarding code review issues

[Emily Shaffer]

On Fri, Sep 13, 2019 at 12:46 PM Ed Tanous <ed.tanous@intel.com> wrote:
>
> On 9/13/19 11:52 AM, Wilfred Smith wrote:
> >
> > I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
> >
> https://github.com/openbmc/docs
>
> > But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
> holds to an 80-column rule instead of 132 in a day of 4K monitors and
> 75-character class names inside nested namespaces that are just as long,
> not counting fully decorated traits). Oh…right…just use “auto”.
>
> From the coding standard here:
> https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md
>
> "Line length should be limited to 80 characters."
>
> You are welcome to disagree with that statement, but the recommended way
> to go about that would be to push a review to change to that rule,
> invoking a 132 character line length, and start a discussion about the
> above.  After that, follow up with changes to the clang-format, and
> commits to each repository to change the codebase to obey the new rule.
> I'm willing to bet if you did that, many people would agree with you,
> and we'd have a very productive discussion about it.

I'll point out that as I understand it, 80c line limit is an
accessibility issue for tools like braille readers or for users who
need to use a larger font size than you may expect.

> Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
> character line lengths) the CI could give faster feedback for obvious
> things.  Is that something you'd be willing to look into automating in
> the CI?  I'm sure a lot of people would appreciate you doing so.
> There's a lot of low hanging fruit here that could be added to CI to
> make the responses seem less hostile.
>>
> It should be noted, for most code, all of this formatting is automatic
> via clang-format, which is documented on the next line in the file I linked.
>
> With all of the above, I'm struggling to see how you're wanting to
> improve here.  Do you think the documentation could be cleaned up?  Is
> the documentation too long to read?  Should we organize it in a
> different way so that the information above is easier to find?
>
> >
> > The process should be somewhat predictable, preferably programmatic.
>
> Agreed, but throughout this email, I don't see any actionable suggestion
> on how this could be improved.  Could you focus a little more on what
> you think we could change to make this situation better?
>
> >
> > Mind you, I have no problem with complying with the maintainer’s rules, but I’d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?

I don't want to be a killjoy here, but I think that's an overly
ambitious goal. I see very, very few code reviews pass with no
complaints on the first try - in this project and in other open source
projects in the wild. I think you may need to dial back your
expectations a little on that front. A more reasonable goal may be
"nit comments only", for example.

>
> Assuming this is the review you're talking about:
> https://gerrit.openbmc-project.xyz/c/openbmc/meta-facebook/+/25145
>
> It was 7 lines of bitbake config, and it followed some project practices
> that are deprecated and trying to be removed.
>
> Interestingly enough, I'm not able to find where we document the 50/72
> rule for commit messages, although I know some editors will enforce it
> when you do your edits.  I would've expected it to be here:
> https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
> Maybe that's an improvement to the docs you could propose?
>
> >
> > It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
>
> >
> > Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
>
> At the end of the day, the commit message is what most people are
> looking at once the commit is merged, and you put a single line that
> wasn't very descriptive.  The maintainer pointed out that you already
> had the level of detail needed, you just failed to put it in the commit
> message.  Certainly, writing this email took longer than just updating
> the commit message with more detail?

I wonder about this too. In my mind, posting an update based on a code
reviewer's comment - especially for the trivial stuff like "run
clang-format" or "reword the commit message".  If you're finding the
process of updating a code review with comments to involve a lot of
overhead, please cry out for help in IRC - it doesn't have to be :) In
fact, (putting on Git contributor hat) I'll personally volunteer to
answer any and all Git questions you may have via email or IRC PM.


Finally, I'd like to encourage you to think of code reviews not as
"the maintainers are grading my work", but instead as "I am but one
human; together we can do more."  Open source is collaborative in
nature, and a really large part of that is code review - all patches
in open source projects are the result of collaboration between the
primary author and the reviewers, almost all the time. I hope you can
see comments as "here is a thing you may not have known," rather than
"ya big dummy!"

 - Emily

Re: Rant regarding code review issues

[Ed Tanous]

On 9/13/19 1:02 PM, Emily Shaffer wrote:
> I'll point out that as I understand it, 80c line limit is an
> accessibility issue for tools like braille readers or for users who
> need to use a larger font size than you may expect.

Neat.  I never knew that was a reasoning for 80 character widths.

I will probably continue to complain in my own head every time I need to
go to a few nested scopes and my line gets wrapped in an inconvenient
place, but to a lesser extent than before now that I know there's a good
reason for it :)

-Ed

Re: Rant regarding code review issues

[Brad Bishop]

at 4:10 PM, Ed Tanous <ed.tanous@intel.com> wrote:

> On 9/13/19 1:02 PM, Emily Shaffer wrote:
>> I'll point out that as I understand it, 80c line limit is an
>> accessibility issue for tools like braille readers or for users who
>> need to use a larger font size than you may expect.
>
> Neat.  I never knew that was a reasoning for 80 character widths.

Me either…TIL...

>
> I will probably continue to complain in my own head every time I need to
> go to a few nested scopes and my line gets wrapped in an inconvenient
> place, but to a lesser extent than before now that I know there's a good
> reason for it :)

Another commonly sited reason for 80c + 8space indent… highlighting when  
you have too many levels of nested scopes :-)

Re: Rant regarding code review issues

[Johnathan Mantey]

[-- Attachment #1.1.1: Type: text/plain, Size: 7168 bytes --]

I watched a presentation online where the presenter used long lines.  It
was impossible to understand his content because of the long lines.  He
was constantly scrolling left/right, effectively obfuscating the code. 

Likewise, if you're going to cut code into a presentation slide deck it
requires no effort to get it to fit on the slide when the line length is
80c non-proportional.

I prefer 80c/line because it requires less eye motion to read. 
It requires less hand motion, in the event you have to scroll left/right.
It reduces the need to mentally unroll a line should you have line
wrapping enabled, and the line exceeds your screen/display width.
It reduces the need to mentally combine a line in the event you are
scrolling right/left.

Sadly C++ encourages long lines because of its verbosity.

On 9/13/19 1:02 PM, Emily Shaffer wrote:
> On Fri, Sep 13, 2019 at 12:46 PM Ed Tanous <ed.tanous@intel.com> wrote:
>> On 9/13/19 11:52 AM, Wilfred Smith wrote:
>>> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
>>>
>> https://github.com/openbmc/docs
>>
>>> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
>> holds to an 80-column rule instead of 132 in a day of 4K monitors and
>> 75-character class names inside nested namespaces that are just as long,
>> not counting fully decorated traits). Oh…right…just use “auto”.
>>
>> From the coding standard here:
>> https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md
>>
>> "Line length should be limited to 80 characters."
>>
>> You are welcome to disagree with that statement, but the recommended way
>> to go about that would be to push a review to change to that rule,
>> invoking a 132 character line length, and start a discussion about the
>> above.  After that, follow up with changes to the clang-format, and
>> commits to each repository to change the codebase to obey the new rule.
>> I'm willing to bet if you did that, many people would agree with you,
>> and we'd have a very productive discussion about it.
> I'll point out that as I understand it, 80c line limit is an
> accessibility issue for tools like braille readers or for users who
> need to use a larger font size than you may expect.
>
>> Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
>> character line lengths) the CI could give faster feedback for obvious
>> things.  Is that something you'd be willing to look into automating in
>> the CI?  I'm sure a lot of people would appreciate you doing so.
>> There's a lot of low hanging fruit here that could be added to CI to
>> make the responses seem less hostile.
>> It should be noted, for most code, all of this formatting is automatic
>> via clang-format, which is documented on the next line in the file I linked.
>>
>> With all of the above, I'm struggling to see how you're wanting to
>> improve here.  Do you think the documentation could be cleaned up?  Is
>> the documentation too long to read?  Should we organize it in a
>> different way so that the information above is easier to find?
>>
>>> The process should be somewhat predictable, preferably programmatic.
>> Agreed, but throughout this email, I don't see any actionable suggestion
>> on how this could be improved.  Could you focus a little more on what
>> you think we could change to make this situation better?
>>
>>> Mind you, I have no problem with complying with the maintainer’s rules, but I’d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?
> I don't want to be a killjoy here, but I think that's an overly
> ambitious goal. I see very, very few code reviews pass with no
> complaints on the first try - in this project and in other open source
> projects in the wild. I think you may need to dial back your
> expectations a little on that front. A more reasonable goal may be
> "nit comments only", for example.
>
>> Assuming this is the review you're talking about:
>> https://gerrit.openbmc-project.xyz/c/openbmc/meta-facebook/+/25145
>>
>> It was 7 lines of bitbake config, and it followed some project practices
>> that are deprecated and trying to be removed.
>>
>> Interestingly enough, I'm not able to find where we document the 50/72
>> rule for commit messages, although I know some editors will enforce it
>> when you do your edits.  I would've expected it to be here:
>> https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
>> Maybe that's an improvement to the docs you could propose?
>>
>>> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
>>> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
>> At the end of the day, the commit message is what most people are
>> looking at once the commit is merged, and you put a single line that
>> wasn't very descriptive.  The maintainer pointed out that you already
>> had the level of detail needed, you just failed to put it in the commit
>> message.  Certainly, writing this email took longer than just updating
>> the commit message with more detail?
> I wonder about this too. In my mind, posting an update based on a code
> reviewer's comment - especially for the trivial stuff like "run
> clang-format" or "reword the commit message".  If you're finding the
> process of updating a code review with comments to involve a lot of
> overhead, please cry out for help in IRC - it doesn't have to be :) In
> fact, (putting on Git contributor hat) I'll personally volunteer to
> answer any and all Git questions you may have via email or IRC PM.
>
>
> Finally, I'd like to encourage you to think of code reviews not as
> "the maintainers are grading my work", but instead as "I am but one
> human; together we can do more."  Open source is collaborative in
> nature, and a really large part of that is code review - all patches
> in open source projects are the result of collaboration between the
> primary author and the reviewers, almost all the time. I hope you can
> see comments as "here is a thing you may not have known," rather than
> "ya big dummy!"
>
>  - Emily

-- 
Johnathan Mantey
Senior Software Engineer
*azad te**chnology partners*
Contributing to Technology Innovation since 1992
Phone: (503) 712-6764
Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>


[-- Attachment #1.1.2: Type: text/html, Size: 10103 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Rant regarding code review issues

[Wilfred Smith]

Please, everyone, again my sincere apologies for upsetting anyone. I was frustrated and sought commiseration.

I will endeavor to push content that addresses my concerns; hopefully that will serve the community better.

I do welcome these comments and will take them to heart. This is my first venture into open source IP. I think your patience will eventually be a worthwhile investment.

Wilfred




> On Sep 13, 2019, at 1:54 PM, openbmc-request@lists.ozlabs.org wrote:
> 
> Send openbmc mailing list submissions to
> 	openbmc@lists.ozlabs.org
> 
> To subscribe or unsubscribe via the World Wide Web, visit
> 	https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_listinfo_openbmc&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=s2_Ear18pkauQy8F_eaQKH620jl0g6IUzDXnSUrXyzI&e= 
> or, via email, send a message with subject or body 'help' to
> 	openbmc-request@lists.ozlabs.org
> 
> You can reach the person managing the list at
> 	openbmc-owner@lists.ozlabs.org
> 
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of openbmc digest..."
> 
> 
> Today's Topics:
> 
>   1. Re: Rant regarding code review issues

> (Ed Tanous)
>   2. Re: Rant regarding code review issues (Emily Shaffer)
>   3. Re: Rant regarding code review issues (Ed Tanous)
>   4. Re: Rant regarding code review issues (Brad Bishop)
>   5. Re: Rant regarding code review issues (Johnathan Mantey)
> 
> 
> ----------------------------------------------------------------------
> 
> Message: 1
> Date: Fri, 13 Sep 2019 12:45:25 -0700
> From: Ed Tanous <ed.tanous@intel.com>
> To: openbmc@lists.ozlabs.org
> Subject: Re: Rant regarding code review issues
> Message-ID: <f5205bbe-7320-67ee-2b2d-91c5cc185b02@intel.com>
> Content-Type: text/plain; charset=utf-8
> 
> On 9/13/19 11:52 AM, Wilfred Smith wrote:
>> 
>> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
>> 
> https://github.com/openbmc/docs
> 
>> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
> holds to an 80-column rule instead of 132 in a day of 4K monitors and
> 75-character class names inside nested namespaces that are just as long,
> not counting fully decorated traits). Oh?right?just use ?auto?.
> 
>> From the coding standard here:
> https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md
> 
> "Line length should be limited to 80 characters."
> 
> You are welcome to disagree with that statement, but the recommended way
> to go about that would be to push a review to change to that rule,
> invoking a 132 character line length, and start a discussion about the
> above.  After that, follow up with changes to the clang-format, and
> commits to each repository to change the codebase to obey the new rule.
> I'm willing to bet if you did that, many people would agree with you,
> and we'd have a very productive discussion about it.
> 
> It should be noted, for most code, all of this formatting is automatic
> via clang-format, which is documented on the next line in the file I linked.
> 
> With all of the above, I'm struggling to see how you're wanting to
> improve here.  Do you think the documentation could be cleaned up?  Is
> the documentation too long to read?  Should we organize it in a
> different way so that the information above is easier to find?
> 
>> 
>> The process should be somewhat predictable, preferably programmatic.
> 
> Agreed, but throughout this email, I don't see any actionable suggestion
> on how this could be improved.  Could you focus a little more on what
> you think we could change to make this situation better?
> 
>> 
>> Mind you, I have no problem with complying with the maintainer?s rules, but I?d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?
> 
> Assuming this is the review you're talking about:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_meta-2Dfacebook_-2B_25145&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=x2eMFtQOCcaJ5Tq6rd6rIHLNYxxKPs72kcZyWQvxlvQ&e= 
> 
> It was 7 lines of bitbake config, and it followed some project practices
> that are deprecated and trying to be removed.
> 
> Interestingly enough, I'm not able to find where we document the 50/72
> rule for commit messages, although I know some editors will enforce it
> when you do your edits.  I would've expected it to be here:
> https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
> Maybe that's an improvement to the docs you could propose?
> 
>> 
>> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
> 
> Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
> character line lengths) the CI could give faster feedback for obvious
> things.  Is that something you'd be willing to look into automating in
> the CI?  I'm sure a lot of people would appreciate you doing so.
> There's a lot of low hanging fruit here that could be added to CI to
> make the responses seem less hostile.
> 
>> 
>> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
> 
> At the end of the day, the commit message is what most people are
> looking at once the commit is merged, and you put a single line that
> wasn't very descriptive.  The maintainer pointed out that you already
> had the level of detail needed, you just failed to put it in the commit
> message.  Certainly, writing this email took longer than just updating
> the commit message with more detail?
> 
> 
> ------------------------------
> 
> Message: 2
> Date: Fri, 13 Sep 2019 13:02:07 -0700
> From: Emily Shaffer <emilyshaffer@google.com>
> To: Ed Tanous <ed.tanous@intel.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: Re: Rant regarding code review issues
> Message-ID:
> 	<CAJoAoZm2apzNtkLNHULG1pkUL4U27+DoeVeR9-fgDPyjrBBakw@mail.gmail.com>
> Content-Type: text/plain; charset="UTF-8"
> 
> On Fri, Sep 13, 2019 at 12:46 PM Ed Tanous <ed.tanous@intel.com> wrote:
>> 
>> On 9/13/19 11:52 AM, Wilfred Smith wrote:
>>> 
>>> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
>>> 
>> https://github.com/openbmc/docs
>> 
>>> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
>> holds to an 80-column rule instead of 132 in a day of 4K monitors and
>> 75-character class names inside nested namespaces that are just as long,
>> not counting fully decorated traits). Oh?right?just use ?auto?.
>> 
>> From the coding standard here:
>> https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md
>> 
>> "Line length should be limited to 80 characters."
>> 
>> You are welcome to disagree with that statement, but the recommended way
>> to go about that would be to push a review to change to that rule,
>> invoking a 132 character line length, and start a discussion about the
>> above.  After that, follow up with changes to the clang-format, and
>> commits to each repository to change the codebase to obey the new rule.
>> I'm willing to bet if you did that, many people would agree with you,
>> and we'd have a very productive discussion about it.
> 
> I'll point out that as I understand it, 80c line limit is an
> accessibility issue for tools like braille readers or for users who
> need to use a larger font size than you may expect.
> 
>> Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
>> character line lengths) the CI could give faster feedback for obvious
>> things.  Is that something you'd be willing to look into automating in
>> the CI?  I'm sure a lot of people would appreciate you doing so.
>> There's a lot of low hanging fruit here that could be added to CI to
>> make the responses seem less hostile.
>>> 
>> It should be noted, for most code, all of this formatting is automatic
>> via clang-format, which is documented on the next line in the file I linked.
>> 
>> With all of the above, I'm struggling to see how you're wanting to
>> improve here.  Do you think the documentation could be cleaned up?  Is
>> the documentation too long to read?  Should we organize it in a
>> different way so that the information above is easier to find?
>> 
>>> 
>>> The process should be somewhat predictable, preferably programmatic.
>> 
>> Agreed, but throughout this email, I don't see any actionable suggestion
>> on how this could be improved.  Could you focus a little more on what
>> you think we could change to make this situation better?
>> 
>>> 
>>> Mind you, I have no problem with complying with the maintainer?s rules, but I?d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?
> 
> I don't want to be a killjoy here, but I think that's an overly
> ambitious goal. I see very, very few code reviews pass with no
> complaints on the first try - in this project and in other open source
> projects in the wild. I think you may need to dial back your
> expectations a little on that front. A more reasonable goal may be
> "nit comments only", for example.
> 
>> 
>> Assuming this is the review you're talking about:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_meta-2Dfacebook_-2B_25145&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=x2eMFtQOCcaJ5Tq6rd6rIHLNYxxKPs72kcZyWQvxlvQ&e= 
>> 
>> It was 7 lines of bitbake config, and it followed some project practices
>> that are deprecated and trying to be removed.
>> 
>> Interestingly enough, I'm not able to find where we document the 50/72
>> rule for commit messages, although I know some editors will enforce it
>> when you do your edits.  I would've expected it to be here:
>> https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
>> Maybe that's an improvement to the docs you could propose?
>> 
>>> 
>>> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
>> 
>>> 
>>> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
>> 
>> At the end of the day, the commit message is what most people are
>> looking at once the commit is merged, and you put a single line that
>> wasn't very descriptive.  The maintainer pointed out that you already
>> had the level of detail needed, you just failed to put it in the commit
>> message.  Certainly, writing this email took longer than just updating
>> the commit message with more detail?
> 
> I wonder about this too. In my mind, posting an update based on a code
> reviewer's comment - especially for the trivial stuff like "run
> clang-format" or "reword the commit message".  If you're finding the
> process of updating a code review with comments to involve a lot of
> overhead, please cry out for help in IRC - it doesn't have to be :) In
> fact, (putting on Git contributor hat) I'll personally volunteer to
> answer any and all Git questions you may have via email or IRC PM.
> 
> 
> Finally, I'd like to encourage you to think of code reviews not as
> "the maintainers are grading my work", but instead as "I am but one
> human; together we can do more."  Open source is collaborative in
> nature, and a really large part of that is code review - all patches
> in open source projects are the result of collaboration between the
> primary author and the reviewers, almost all the time. I hope you can
> see comments as "here is a thing you may not have known," rather than
> "ya big dummy!"
> 
> - Emily
> 
> 
> ------------------------------
> 
> Message: 3
> Date: Fri, 13 Sep 2019 13:10:05 -0700
> From: Ed Tanous <ed.tanous@intel.com>
> To: Emily Shaffer <emilyshaffer@google.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: Re: Rant regarding code review issues
> Message-ID: <50cbafb6-6043-40fa-ef2c-2289906afecd@intel.com>
> Content-Type: text/plain; charset=utf-8
> 
> On 9/13/19 1:02 PM, Emily Shaffer wrote:
>> I'll point out that as I understand it, 80c line limit is an
>> accessibility issue for tools like braille readers or for users who
>> need to use a larger font size than you may expect.
> 
> Neat.  I never knew that was a reasoning for 80 character widths.
> 
> I will probably continue to complain in my own head every time I need to
> go to a few nested scopes and my line gets wrapped in an inconvenient
> place, but to a lesser extent than before now that I know there's a good
> reason for it :)
> 
> -Ed
> 
> 
> ------------------------------
> 
> Message: 4
> Date: Fri, 13 Sep 2019 16:27:47 -0400
> From: Brad Bishop <bradleyb@fuzziesquirrel.com>
> To: Ed Tanous <ed.tanous@intel.com>
> Cc: Emily Shaffer <emilyshaffer@google.com>, OpenBMC Maillist
> 	<openbmc@lists.ozlabs.org>
> Subject: Re: Rant regarding code review issues
> Message-ID: <BB11D70B-19A2-467B-9C9B-98A04F33EF39@fuzziesquirrel.com>
> Content-Type: text/plain;	charset=utf-8;	delsp=yes;	format=flowed
> 
> at 4:10 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> 
>> On 9/13/19 1:02 PM, Emily Shaffer wrote:
>>> I'll point out that as I understand it, 80c line limit is an
>>> accessibility issue for tools like braille readers or for users who
>>> need to use a larger font size than you may expect.
>> 
>> Neat.  I never knew that was a reasoning for 80 character widths.
> 
> Me either?TIL...
> 
>> 
>> I will probably continue to complain in my own head every time I need to
>> go to a few nested scopes and my line gets wrapped in an inconvenient
>> place, but to a lesser extent than before now that I know there's a good
>> reason for it :)
> 
> Another commonly sited reason for 80c + 8space indent? highlighting when  
> you have too many levels of nested scopes :-)
> 
> 
> ------------------------------
> 
> Message: 5
> Date: Fri, 13 Sep 2019 13:53:46 -0700
> From: Johnathan Mantey <johnathanx.mantey@intel.com>
> To: Emily Shaffer <emilyshaffer@google.com>, Ed Tanous
> 	<ed.tanous@intel.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: Re: Rant regarding code review issues
> Message-ID: <c15e0e55-bc0f-fbeb-2250-20330a87a50a@intel.com>
> Content-Type: text/plain; charset="utf-8"
> 
> I watched a presentation online where the presenter used long lines.? It
> was impossible to understand his content because of the long lines.? He
> was constantly scrolling left/right, effectively obfuscating the code.?
> 
> Likewise, if you're going to cut code into a presentation slide deck it
> requires no effort to get it to fit on the slide when the line length is
> 80c non-proportional.
> 
> I prefer 80c/line because it requires less eye motion to read.?
> It requires less hand motion, in the event you have to scroll left/right.
> It reduces the need to mentally unroll a line should you have line
> wrapping enabled, and the line exceeds your screen/display width.
> It reduces the need to mentally combine a line in the event you are
> scrolling right/left.
> 
> Sadly C++ encourages long lines because of its verbosity.
> 
> On 9/13/19 1:02 PM, Emily Shaffer wrote:
>> On Fri, Sep 13, 2019 at 12:46 PM Ed Tanous <ed.tanous@intel.com> wrote:
>>> On 9/13/19 11:52 AM, Wilfred Smith wrote:
>>>> I strongly assert that there should be a formal list of commit rules. If the rules were actually documented and published, one could go through the checklist to ensure that the commit conforms. The result would be less time wasted by the committer and the reviewers, as well as more consistency.
>>>> 
>>> https://github.com/openbmc/docs
>>> 
>>>> But it sucks to have a commit booted for reasons that (a) I could not reasonably be expected to have known previously (because I read the documentation, of course) and(b) I may legitimately disagree with (e.g. the insanity that anyone
>>> holds to an 80-column rule instead of 132 in a day of 4K monitors and
>>> 75-character class names inside nested namespaces that are just as long,
>>> not counting fully decorated traits). Oh?right?just use ?auto?.
>>> 
>>> From the coding standard here:
>>> https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md
>>> 
>>> "Line length should be limited to 80 characters."
>>> 
>>> You are welcome to disagree with that statement, but the recommended way
>>> to go about that would be to push a review to change to that rule,
>>> invoking a 132 character line length, and start a discussion about the
>>> above.  After that, follow up with changes to the clang-format, and
>>> commits to each repository to change the codebase to obey the new rule.
>>> I'm willing to bet if you did that, many people would agree with you,
>>> and we'd have a very productive discussion about it.
>> I'll point out that as I understand it, 80c line limit is an
>> accessibility issue for tools like braille readers or for users who
>> need to use a larger font size than you may expect.
>> 
>>> Agreed.  I'd also mention that in some cases (like the 50/27 rule and 80
>>> character line lengths) the CI could give faster feedback for obvious
>>> things.  Is that something you'd be willing to look into automating in
>>> the CI?  I'm sure a lot of people would appreciate you doing so.
>>> There's a lot of low hanging fruit here that could be added to CI to
>>> make the responses seem less hostile.
>>> It should be noted, for most code, all of this formatting is automatic
>>> via clang-format, which is documented on the next line in the file I linked.
>>> 
>>> With all of the above, I'm struggling to see how you're wanting to
>>> improve here.  Do you think the documentation could be cleaned up?  Is
>>> the documentation too long to read?  Should we organize it in a
>>> different way so that the information above is easier to find?
>>> 
>>>> The process should be somewhat predictable, preferably programmatic.
>>> Agreed, but throughout this email, I don't see any actionable suggestion
>>> on how this could be improved.  Could you focus a little more on what
>>> you think we could change to make this situation better?
>>> 
>>>> Mind you, I have no problem with complying with the maintainer?s rules, but I?d like to have some chance at getting a trivial commit passed on the first shot. Two lines of BitBake config, that actually accomplish what is intended, fetches a half-dozen complaints?
>> I don't want to be a killjoy here, but I think that's an overly
>> ambitious goal. I see very, very few code reviews pass with no
>> complaints on the first try - in this project and in other open source
>> projects in the wild. I think you may need to dial back your
>> expectations a little on that front. A more reasonable goal may be
>> "nit comments only", for example.
>> 
>>> Assuming this is the review you're talking about:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__gerrit.openbmc-2Dproject.xyz_c_openbmc_meta-2Dfacebook_-2B_25145&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=x2eMFtQOCcaJ5Tq6rd6rIHLNYxxKPs72kcZyWQvxlvQ&e= 
>>> 
>>> It was 7 lines of bitbake config, and it followed some project practices
>>> that are deprecated and trying to be removed.
>>> 
>>> Interestingly enough, I'm not able to find where we document the 50/72
>>> rule for commit messages, although I know some editors will enforce it
>>> when you do your edits.  I would've expected it to be here:
>>> https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes
>>> Maybe that's an improvement to the docs you could propose?
>>> 
>>>> It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
>>>> Also, there are too many places to put the same information. I quoted the warning messages I was remedying in the patch set comments, but summarized in the commit message because that seemed like the right thing to do. And that got me another downvote.
>>> At the end of the day, the commit message is what most people are
>>> looking at once the commit is merged, and you put a single line that
>>> wasn't very descriptive.  The maintainer pointed out that you already
>>> had the level of detail needed, you just failed to put it in the commit
>>> message.  Certainly, writing this email took longer than just updating
>>> the commit message with more detail?
>> I wonder about this too. In my mind, posting an update based on a code
>> reviewer's comment - especially for the trivial stuff like "run
>> clang-format" or "reword the commit message".  If you're finding the
>> process of updating a code review with comments to involve a lot of
>> overhead, please cry out for help in IRC - it doesn't have to be :) In
>> fact, (putting on Git contributor hat) I'll personally volunteer to
>> answer any and all Git questions you may have via email or IRC PM.
>> 
>> 
>> Finally, I'd like to encourage you to think of code reviews not as
>> "the maintainers are grading my work", but instead as "I am but one
>> human; together we can do more."  Open source is collaborative in
>> nature, and a really large part of that is code review - all patches
>> in open source projects are the result of collaboration between the
>> primary author and the reviewers, almost all the time. I hope you can
>> see comments as "here is a thing you may not have known," rather than
>> "ya big dummy!"
>> 
>> - Emily
> 
> -- 
> Johnathan Mantey
> Senior Software Engineer
> *azad te**chnology partners*
> Contributing to Technology Innovation since 1992
> Phone: (503) 712-6764
> Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>
> 
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.ozlabs.org_pipermail_openbmc_attachments_20190913_a21a342e_attachment.htm&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=nFvHrZFPIKcnWWbbfK5dc8knK3wzHwYbNc_HcMxCWow&e= >
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: signature.asc
> Type: application/pgp-signature
> Size: 488 bytes
> Desc: OpenPGP digital signature
> URL: <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.ozlabs.org_pipermail_openbmc_attachments_20190913_a21a342e_attachment.sig&d=DwICAg&c=5VD0RTtNlTh3ycd41b3MUw&r=-ektT-tD9zf2rfUisE63RqiDagGyhGey2hbEGa-47kc&m=12QUGCNeKzJaeya2socKZls_OsPkE4dQVz5ydFylcr4&s=013H58TIHWJAyfZPR2D7jt3SuZ8vGLhPuF5_wW1hRxA&e= >
> 
> End of openbmc Digest, Vol 49, Issue 81
> ***************************************

Re: Rant regarding code review issues

[Andrew Jeffery]

Hi Wilfred,

On Sat, 14 Sep 2019, at 08:46, Wilfred Smith wrote:
> Please, everyone, again my sincere apologies for upsetting anyone. I 
> was frustrated and sought commiseration.

An immediate problem I have with your approach is that you're ranting at
the people that you're seeking commiseration from. I don't understand
how you expect that to work unless you're trying to exploit cracks in the
community.

> 
> I will endeavor to push content that addresses my concerns; hopefully 
> that will serve the community better.

That would be appreciated. Please be aware that the outcome *may*
be along the lines of "thanks for your concern, however we can't
support your proposal for reasons X, Y and Z". There may be paths to
resolve X, Y and Z, and if so, please try to work constructively with the
people that raise them.

> 
> I do welcome these comments and will take them to heart. This is my 
> first venture into open source IP. I think your patience will 
> eventually be a worthwhile investment.

People come and go in open-source communities with many timings and
for many reasons. There's no stability in an open source community beyond
what individuals put in. In general we can't expect that anyone is going to
be present and working on ideas from one day to the next. The consequence
is that it's a personal choice for each community member as to how much
they invest in supporting other members of the community.

How you engage with the community will determine how quickly and
completely its existing members build trust in you - we can only make
judgement on that by how you interact on the mailing list, IRC and through
code review. You build my trust in you by rocking up consistently over time,
with interesting patches, are responsive to reviews and are willing to iterate
in response to reasonable requests. Your goal should be to achieve rough
consensus on a patch and get it merged, especially when you are new to the
community and even if some reviews seem unreasonable (though feel free to
ask for justification). Over time as trust builds in you, if you disagree with
particulars of how the community runs then you can start proposing that the
community change to meet your needs, but be aware that there might be
competing needs that drove the existence of the current conditions.

In summary, it's down to each individual in the community to choose
whether they have patience, whether and how much they want to invest, and
how quickly you're progressing along their worthwhile-investment trajectory.
Try to do things that will keep you on track for everyone in the community,
especially in the early days of your participation here.

Cheers,

Andrew