+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.
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
