Thank you all for your clarifications. I'm about to send updated revisions of all patches. I made a rebase and fixed all indentation/alignment issues across all patches.
Thanks. On Wed, 18 May 2016 15:31:32 +0100 Daniel Stone <[email protected]> wrote: > Hi, > > On 18 May 2016 at 15:25, Derek Foreman <[email protected]> wrote: > > On 18/05/16 08:41 AM, Mike Blumenkrantz wrote: > >> In fairness, we'd likely be less short on review bandwidth if the > >> majority of that bandwidth was not in use to make/revise trivial > >> criticisms such as whitespace usage and comment grammar which are > >> guaranteed to be cleaned up in future patches; this leads to > >> burnout on both the code-writing side as well as the reviewing > >> side since everyone has become hyper attuned to the insignificant > >> and non-functional minutiae of patches rather than focusing more > >> on expediting the technical development of the protocol. > > > > Fair points, though I'm not certain "will certainly get fixed up > > later" is a given. Certainly indenting and basic style is a > > mechanical problem that could be tested pre-commit hooks, and there > > should be no reason to bike shed that on the list at all. > > > > Grammar probably needs more serious consideration for protocol doc > > than elsewhere due to its potential impact on compositor > > implementors - but ever there probably not to the degree we're > > seeing lately. > > > > Follow up commits that do nothing but change style and grammar can > > make "git blame" less useful (when trying to figure out who would > > best review a piece of code - not just "arrrrgh who wrote this > > stupid bug") and provide churn for very little benefit, imho. > > Yeah, I agree. I get that the bikeshedding can be annoying; I do (for > that reason, if no other) like tagging commits as 'RFC' or similar, > which is effectively, 'please just check out the technical concept and > don't worry about memory leaks or spelling mistakes right now'. But > given that it's pretty trivial to fix up, and you're likely to have to > rebase _anyway_, I don't see the harm in doing one round of review for > clarity. > > Generally, there's no need to send out a subsequent revision round > just because someone has noted some typos - send it again if you need > a resend anyway to get people to pick it up after a rebase, or if > there have been notable changes, but you shouldn't be arriving at v17 > just because you have difficulty spelling. > > Similarly, 'no, I disagree' is a reasonable response to someone > bikeshedding your exact choice of variables or naming. Review is meant > to be a discussion, not something you just have to unilaterally > acqiuesce to. > > > While we're drifting just slightly off topic here, I'm also > > concerned about the basic usage of some of our tags: > > > > Reviewed-by: indicates rigorous technical review *AND* a firm > > conviction that the feature is important and should be included. > > > > Acked-by: Indicates a firm conviction that the feature is important > > and should be included, but no rigorous review has taken place. > > > > Signed-off-by: Indicates an assumption of responsibility for the > > code. > > > > I've seen a lot of Reviewed-by that I think is just "I looked at the > > spelling and you're good to go". I'm starting to find this > > disconcerting. > > Yes, completely agreed. There's a huge gulf between 'I looked at this > and nothing bad jumped out', versus 'I understand the implications > with this and am prepared to put my name down in agreeance with it > being a good idea'. I am pretty bad with review latency in general, > but on the other hand I do at least give them a reasonably thorough > look over ... > > Cheers, > Daniel -- Miguel _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
