<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, May 18, 2016 at 6:33 PM Yong Bakos <<a href="mailto:junk@humanoriented.com">junk@humanoriented.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
I'm partly to blame for the bikeshedding on writing, yet believe me,<br>
I'm always asking "is this worth it?", prioritizing corrections/clarity<br>
over mere style judgements, and am definitely sensitive to<br>
everyone's cognitive bandwidth. My goal is high quality documentation<br>
for Wayland, and I am open to suggestions on how to best deliver that.<br>
<br>
(Maybe we can chat about this over IRC rather than further hijack this thread.)<br></blockquote><div><br></div><div>Just to be clear, I think your work is worthwhile and valuable irregardless of when in the development cycle it takes place. Without someone taking an interest in alot of these matters, the codebase and documentation would be literally unreadable, and I'm sure its not just myself who feels this way.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On May 18, 2016, at 10:31 AM, Daniel Stone <<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</a>> wrote:<br>
><br>
> Hi,<br>
><br>
> On 18 May 2016 at 15:25, Derek Foreman <<a href="mailto:derekf@osg.samsung.com" target="_blank">derekf@osg.samsung.com</a>> wrote:<br>
>> On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:<br>
>>> In fairness, we'd likely be less short on review bandwidth if the<br>
>>> majority of that bandwidth was not in use to make/revise trivial<br>
>>> criticisms such as whitespace usage and comment grammar which are<br>
>>> guaranteed to be cleaned up in future patches; this leads to burnout on<br>
>>> both the code-writing side as well as the reviewing side since everyone<br>
>>> has become hyper attuned to the insignificant and non-functional<br>
>>> minutiae of patches rather than focusing more on expediting the<br>
>>> technical development of the protocol.<br>
<br>
I agree with the crux of Mike's point. Yet, should trivial correctness<br>
be revised before merging or should post-merge corrections clutter the<br>
blame history? (Rhetorical question.)<br></blockquote><div><br></div><div>I'm more in favor of doing it post-merge even if it does add more entries to the history. Wayland as a whole has a small number of people working on and specializing in different areas, and the net result is that there's relatively few people who are able to review/develop these areas. Bogging down technical progress in order to perfect non-technical aspects will only make it more frustrating for everyone involved and probably reduce future interest in participating in the project.</div><div><br></div><div>Put more simply, it's much less hassle to locally `git blame` something twice to adjust for cleanups than to submit and review a second draft of a patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
>> Fair points, though I'm not certain "will certainly get fixed up later"<br>
>> is a given.  Certainly indenting and basic style is a mechanical problem<br>
>> that could be tested pre-commit hooks, and there should be no reason to<br>
>> bike shed that on the list at all.<br>
<br>
Yes!<br>
<br>
<br>
>> Grammar probably needs more serious consideration for protocol doc than<br>
>> elsewhere due to its potential impact on compositor implementors - but<br>
>> ever there probably not to the degree we're seeing lately.<br>
>><br>
>> Follow up commits that do nothing but change style and grammar can make<br>
>> "git blame" less useful (when trying to figure out who would best review<br>
>> a piece of code - not just "arrrrgh who wrote this stupid bug") and<br>
>> provide churn for very little benefit, imho.<br>
><br>
> Yeah, I agree. I get that the bikeshedding can be annoying; I do (for<br>
> that reason, if no other) like tagging commits as 'RFC' or similar,<br>
> which is effectively, 'please just check out the technical concept and<br>
> don't worry about memory leaks or spelling mistakes right now'. But<br>
> given that it's pretty trivial to fix up, and you're likely to have to<br>
> rebase _anyway_, I don't see the harm in doing one round of review for<br>
> clarity.<br>
><br>
> Generally, there's no need to send out a subsequent revision round<br>
> just because someone has noted some typos - send it again if you need<br>
> a resend anyway to get people to pick it up after a rebase, or if<br>
> there have been notable changes, but you shouldn't be arriving at v17<br>
> just because you have difficulty spelling.<br>
<br>
Perhaps non-technical suggestions should be sent only directly to the<br>
author, for consideration in incorporating in the event of subsequent<br>
revisions, and we not send these to the list? This gives the author<br>
the benefit of the information, and does not clutter the list with<br>
non-technical reviews.<br></blockquote><div><br></div><div>In line with my previous comments, I think providing patches after the fact is sufficient here. These types of patches (outside of protocol-affecting changes) can be reviewed by anyone and don't require specialists in any particular area of the codebase.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> Similarly, 'no, I disagree' is a reasonable response to someone<br>
> bikeshedding your exact choice of variables or naming. Review is meant<br>
> to be a discussion, not something you just have to unilaterally<br>
> acqiuesce to.<br>
><br>
>> While we're drifting just slightly off topic here, I'm also concerned<br>
>> about the basic usage of some of our tags:<br>
>><br>
>> Reviewed-by:  indicates rigorous technical review *AND* a firm<br>
>> conviction that the feature is important and should be included.<br>
>><br>
>> Acked-by: Indicates a firm conviction that the feature is important and<br>
>> should be included, but no rigorous review has taken place.<br>
>><br>
>> Signed-off-by: Indicates an assumption of responsibility for the code.<br>
<br>
The rigor is important. We should add this to:<br>
<a href="https://wayland.freedesktop.org/reviewing.html" rel="noreferrer" target="_blank">https://wayland.freedesktop.org/reviewing.html</a>.<br>
<br>
<br>
>><br>
>> I've seen a lot of Reviewed-by that I think is just "I looked at the<br>
>> spelling and you're good to go".  I'm starting to find this disconcerting.<br>
><br>
> Yes, completely agreed. There's a huge gulf between 'I looked at this<br>
> and nothing bad jumped out', versus 'I understand the implications<br>
> with this and am prepared to put my name down in agreeance with it<br>
> being a good idea'. I am pretty bad with review latency in general,<br>
> but on the other hand I do at least give them a reasonably thorough<br>
> look over ...<br>
><br>
> Cheers,<br>
> Daniel<br>
<br>
Come on Daniel, agreeance is an outdated word that should never be used<br>
in place of agreement. And you misspelled acquiesce above, so thank<br>
goodness that's not part of an identifier. (I'm joking! I'm joking!)<br>
<br>
Thanks for all things Wayland,<br>
yong<br>
<br>
<br></blockquote><div><br></div><div>How dare you. Failing to use a comma when directly addressing someone by name? What is this, amateur hour?! (also joking)</div></div></div>