17.10.2017, 10:17, "Martin Smith" <martin.sm...@qt.io>: > +1 > > I am a member of the Qt documentation team, and I am often included as a > reviewer for code changes that also include changes to qdoc comments. I > always assume I am meant to review only the documentation, so if the > documentation is ok, I give the change a +1 and add a comment that I only > reviewed the documentation. > > Is this the right way to do it? Maybe it should be formalized in the system.
FWIW it's possible to teach Gerrit to require +2 from someone from doc team if commit message has "documentation change" tag [1], but require another +2 to allow submit. [1] Enforceable with sanity bot, or maybe can even be done automatically. > > martin > ________________________________________ > From: Development <development-bounces+martin.smith=qt...@qt-project.org> on > behalf of Viktor Engelmann <viktor.engelm...@qt.io> > Sent: Friday, October 13, 2017 1:04:46 PM > To: development@qt-project.org > Subject: [Development] Speeding up the review process (was: PostgreSQL cross > compile for Pi) > > On the [Interest] mailing list there was a discussion about the > review-process taking to long and we also had multiple discussions about that > at the world summit. I have complained about this myself, so I would like to > start a new thread and collect your thoughts and ideas on how to improve the > situation. > > My suggestions would be > > 1. Allow approving your own commits under certain circumstances. examples: > * when you only changed minor things like a variable name (except in > public API), a string, a comment or qdoc entry > * when you already had a +2 and only changed minor things like a > variable name, a string, a comment or qdoc entry (or more generally: when you > already had a +2 and only did something that is also on this list :-D ) > * when you only increased a timeout in an autotest. Increasing timeouts > is a safe change - it can only solve false negatives and false positives, but > not create false positives or false negatives. > * or more general: when you only made an autotest harder to pass - like > adding a Q_VERIFY, Q_COMPARE, etc. > * when the change is something auto-generated - like you just updated > plugins.qmltypes using qmlplugindump > * when you only changed something in accordance to the guidelines - like > Q_DECL_OVERRIDE -> override > * when you have a certain count of +1's from people who have approver > rights > 2. Towards that last point: I think many of us are afraid to get blamed for > +2'ing something that causes problems later (introduces a new bug or so), but > as far as I have seen, nobody gets blamed for such problems, so we should not > be THAT afraid of approving something. Also, don't forget that there is still > the CI to get past! > 3. Remember that brain-cycles are far more expensive than CPU cycles - so > when in doubt, rather test-run something on the CI than make a human think > about whether the CI "would" accept it. If that causes CI outages, we need to > buy more CI machines. It is just a naive fallacy to "save" CI expenses by > assigning the CI's work to employees who are much more expensive. > 4. I don't think we need to be as paranoid towards contributions from our > own employees as we need to be towards external contributions. > 5. Set a deadline for criticism on the general approach to a change. Too > often I have had the situation that I uploaded a patch, then we debated the > qdoc entries, variable names, method names, etc FOR MONTHS - and when I > thought we were finally wrapping up and I could soon submit it, someone else > chimes in and says "this should be done completely differently". Even if the > person is right - they should have said that months earlier - before I wasted > all that valuable time on variable names, compliance with qdoc guidelines, > etc. > In earlier discussions I have been told that such a deadline would have to be > long, because someone who might have an opinion might be on vacation. IMHO, > this doesn't count, because a) you can still make an exception to the rule in > such a situation and b) by that logic you should never approve anything, > because we also might hire a new employee in 10 years who might have an > opinion. > > -- > > Viktor Engelmann > Software Engineer > > The Qt Company GmbH > Rudower Chaussee 13 > D-12489 Berlin > viktor.engelm...@qt.io<mailto:viktor.engelm...@qt.io> > +49 151 26784521 > http://qt.io > > Geschäftsführer: Mika Pälsi, > Juha Varelius, Mika Harjuaho > Sitz der Gesellschaft: Berlin, > Registergericht: Amtsgericht > Charlottenburg, HRB 144331 B > > The Future is Written with Qt > www.qtworldsummit.com<http://www.qtworldsummit.com> > > _______________________________________________ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development -- Regards, Konstantin _______________________________________________ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
