<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
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. <br>
<br>
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.<br>
<br>
I prefer 80c/line because it requires less eye motion to read. <br>
It requires less hand motion, in the event you have to scroll
left/right.<br>
It reduces the need to mentally unroll a line should you have line
wrapping enabled, and the line exceeds your screen/display width.<br>
It reduces the need to mentally combine a line in the event you are
scrolling right/left.<br>
<br>
Sadly C++ encourages long lines because of its verbosity.<br>
<br>
<div class="moz-cite-prefix">On 9/13/19 1:02 PM, Emily Shaffer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAJoAoZm2apzNtkLNHULG1pkUL4U27+DoeVeR9-fgDPyjrBBakw@mail.gmail.com">
<pre class="moz-quote-pre" wrap="">On Fri, Sep 13, 2019 at 12:46 PM Ed Tanous <a class="moz-txt-link-rfc2396E" href="mailto:ed.tanous@intel.com"><ed.tanous@intel.com></a> wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On 9/13/19 11:52 AM, Wilfred Smith wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><a class="moz-txt-link-freetext" href="https://github.com/openbmc/docs">https://github.com/openbmc/docs</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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:
<a class="moz-txt-link-freetext" href="https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md">https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions.md</a>
"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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
The process should be somewhat predictable, preferably programmatic.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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?
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Assuming this is the review you're talking about:
<a class="moz-txt-link-freetext" href="https://gerrit.openbmc-project.xyz/c/openbmc/meta-facebook/+/25145">https://gerrit.openbmc-project.xyz/c/openbmc/meta-facebook/+/25145</a>
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:
<a class="moz-txt-link-freetext" href="https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes">https://github.com/openbmc/docs/blob/master/CONTRIBUTING.md#submitting-changes</a>
Maybe that's an improvement to the docs you could propose?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
It helps the maintainer if committers are able to self-police 98% of the issues, and makes the entire process seem less hostile.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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
</pre>
</blockquote>
<br>
<div class="moz-signature">-- <br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<title></title>
<font color="#1F497D"><font face="Century Gothic">Johnathan Mantey<br>
<small>Senior Software Engineer</small><br>
<big><font color="#555555"><small><b>azad te</b><b>chnology
partners</b></small><br>
<small><font color="#1F497D"><small>Contributing to
Technology Innovation since 1992</small></font><small><br>
<font color="#1F497D">Phone: (503) 712-6764<br>
Email: <a href="mailto:johnathanx.mantey@intel.com">johnathanx.mantey@intel.com</a></font></small><br>
<br>
</small></font></big></font></font> </div>
</body>
</html>