Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Robert Löhning]

Am 13.10.2017 um 14:07 schrieb Jedrzej Nowacki:
> On piątek, 13 października 2017 13:04:46 CEST Viktor Engelmann wrote:
>> 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:
>>  1. when you only changed minor things like a variable name (except
>> in public API), a string, a comment or qdoc entry
>>  2. 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 )
>>  3. 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.
>>  4. or more general: when you only made an autotest harder to pass -
>> like adding a Q_VERIFY, Q_COMPARE, etc.
>>  5. when the change is something auto-generated - like you just
>> updated plugins.qmltypes using qmlplugindump
>>  6. when you only changed something in accordance to the guidelines
>> - like Q_DECL_OVERRIDE -> override
>>  7. when you have a certain count of +1's from people who have
>> approver rights
> That doesn't solve the problem, no brainers are getting review quite quickly. 
> Some more detailed comments below:
>  - In 2, you would need to amend the commit message too.
>  - 3 is  at best controversial, do not increase timeouts, rewrite the test 
> instead.
>  - I'm not convinced with 4, it may happen that one is enforcing wrong or 
> invalid behaviour.
>  - I'm afraid that as a result of 7 people will not give +1s anymore :-)
> Review as a process is good, it may be annoying but it is proven to increase 
> quality of the code.
> 
>>  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!
> I like how you believe in CI, I share it, but check our test coverage, we are 
> far from perfect.
> 
>>  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.
> The sentence suggests that we have code review process to cut CI expenses, 
> that is simply not true. Anyway, it is not that easy as you wrote there are 
> limits of scalability.

First of all I'd like to thank Viktor for figuring out ways to improve
our review process and getting the discussion going.

While it seems that mostly everything has been said about the other
bullet points, I'd like to comment on points 2 and 3. I get the
impression that these encourage people to trust in CI far too much. My
understanding is that the developers writing code should make sure that
everything they wrote does what they expect. If anything is unclear, the
issue should be discussed with others who know more about the affected
code or its influences. At the time someone stages a change at least
this person and the person giving +2 should be completely convinced that
the patch is free of mistakes. They should act as if no CI ever existed.

Of course, as human beings, we all make mistakes and no matter how
convinced someone is of their work results, they might turn out to be
wrong. To address this, we have the CI, we have RTA, we have manual
tests. These are there to find the remaining errors which slip through.
At least this how I see them. Using them as the judge whether a patch is
good or not, is way beyond what they are intended for. Please correct me
if I'm mistaken.

Cheers,
Robert

> 
>>  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.
> I do not agree. An employer name does not matter.
> 
>>  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, 

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Konstantin Tokarev]

17.10.2017, 16:10, "Kai Koehne" :
>>  -Original Message-
>>  From: Development [mailto:development-bounces+kai.koehne=qt.io@qt-
>>  project.org] On Behalf Of Martin Smith
>>  [...]
>>  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.
>
> Yes, I think this is the right way.
>
> Writing the comment is the key here. We just have -2, -1, +1, +2 available, 
> so a '+1' can mean everything from 'I had barely a look' to 'I just checked 
> parts of it' to 'I think the patch is flawless, but just maybe someone else 
> wants to review it, too' .

It's possible to configure more labels to vote for beyound Code-Review and 
Sanity-Review, see

https://codereview.qt-project.org/Documentation/config-labels.html

>
> So, given the limited options, let's just state explicitly what you mean when 
> giving a review. While at it, consider also to mention positive things, so 
> people don't feel like they only ever get negative feedback.
>
> My 2 cents,
>
> Kai
>
> ___
> 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

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Kai Koehne]

> -Original Message-
> From: Development [mailto:development-bounces+kai.koehne=qt.io@qt-
> project.org] On Behalf Of Martin Smith
> [...]
> 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.

Yes, I think this is the right way. 

Writing the comment is the key here. We just have -2, -1, +1, +2 available, so 
a '+1' can mean everything from 'I had barely a look' to 'I just checked parts 
of it' to 'I think the patch is flawless, but just maybe someone else wants to 
review it, too' .

So, given the limited options, let's just state explicitly what you mean when 
giving a review. While at it, consider also to mention positive things, so 
people don't feel like they only ever get negative feedback.

My 2 cents,

Kai

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Konstantin Tokarev]

17.10.2017, 10:17, "Martin Smith" :
> +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  on 
> behalf of Viktor Engelmann 
> 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
> +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
>
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development

-- 
Regards,
Konstantin

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Martin Smith]

+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  on 
behalf of Viktor Engelmann 
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
+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

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Tuukka Turunen]

Hi,

I hope people can look into Viktor’s complete proposal and think about what is 
good and what not. This is what we should do when someone makes a proposal to 
improve things. I think the idea is very good - being more efficient.

What happened in this thread is taking only some parts of it and starting to 
give completely irrelevant accusations (in some of the earlier responses). 
Result of such behavior is rarely anything good. At least the person who tried 
to improve things feels bad and often the main point gets forgotten.

We had a good discussion at Qt Contributor Summit about code of conduct for the 
Qt Project. I very much hope we are able to create that quickly. It will 
hopefully help people to speak without constant fear of others taking their 
words and starting to beat the person making a suggestion.

Yours,

 Tuukka
_
From: Marc Mutz >
Sent: maanantaina, lokakuuta 16, 2017 12:41 ip.
Subject: Re: [Development] Speeding up the review process (was: PostgreSQL 
cross compile for Pi)
To: >


On 2017-10-13 16:03, Jedrzej Nowacki wrote:
> If you do like that then you are doing it wrong. Review process is
> _not_ based
> on a name / company / sun activity. It is based on the change content.
> Even
> best people do mistakes.

I'm entirely with Jedrzej here. Do not +2 if you don't understand the
change - all of it, regardless of who it's from. If you only did a
cursory review, give a +1, at most, and mention that it's incomplete. If
you do not understand the header change you used as an example, ask for
it to be clarified (in the commit message, preferably), since chances
are that other reviewers or later git history users might have the same
problems understanding code.

I would even qualify what Thiago wrote in response: I don't want
Thiago's +2 on a QStringView patch if he didn't understand the code.
Sure, I don't want unreasonable bikeshedding over subjective matters in
"my" code, since the author of the patch should have the last say in
these, but I don't want someone to usher my code through because I
happen to be the principal author of the code.

Another, slightly related aspect, and one of the reasons why I said
Viktor got it the wrong way around is that I prefer cross-company
reviews over intra-company ones, and so should anyone. If you ask a
member of your own organisation to review a patch, there are social
effects at work that will make the review less valueable than one done
by an outsider: unreasonable trust in your colleagues, trying to be
helpful by unblocking a patch, ...

Consequently, I accept a +2 from a KDABian only if I'm
above-the-ordinary-ly sure about the correctness of my code. Otherwise,
I wait for Thiago or Olivier or any@tqc to give a second opinion.

Yes, it's a pain to wait for code review, but office-neighbour reviews
just tend to produce bad code/apis, and often you can do better by
writing a page of commit message _more_ than you usually would to
anticipate and answer questions regarding the code ahead of time.

Thanks,
Marc



___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development


___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Marc Mutz]

On 2017-10-13 16:03, Jedrzej Nowacki wrote:
If you do like that then you are doing it wrong. Review process is 
_not_ based
on a name / company / sun activity. It is  based on the change content. 
Even

best people do mistakes.


I'm entirely with Jedrzej here. Do not +2 if you don't understand the 
change - all of it, regardless of who it's from. If you only did a 
cursory review, give a +1, at most, and mention that it's incomplete. If 
you do not understand the header change you used as an example, ask for 
it to be clarified (in the commit message, preferably), since chances 
are that other reviewers or later git history users might have the same 
problems understanding code.


I would even qualify what Thiago wrote in response: I don't want 
Thiago's +2 on a QStringView patch if he didn't understand the code. 
Sure, I don't want unreasonable bikeshedding over subjective matters in 
"my" code, since the author of the patch should have the last say in 
these, but I don't want someone to usher my code through because I 
happen to be the principal author of the code.


Another, slightly related aspect, and one of the reasons why I said 
Viktor got it the wrong way around is that I prefer cross-company 
reviews over intra-company ones, and so should anyone. If you ask a 
member of your own organisation to review a patch, there are social 
effects at work that will make the review less valueable than one done 
by an outsider: unreasonable trust in your colleagues, trying to be 
helpful by unblocking a patch, ...


Consequently, I accept a +2 from a KDABian only if I'm 
above-the-ordinary-ly sure about the correctness of my code. Otherwise, 
I wait for Thiago or Olivier or any@tqc to give a second opinion.


Yes, it's a pain to wait for code review, but office-neighbour reviews 
just tend to produce bad code/apis, and often you can do better by 
writing a page of commit message _more_ than you usually would to 
anticipate and answer questions regarding the code ahead of time.


Thanks,
Marc



___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Adam Treat]

+1

I also really appreciate when a reviewer is up front about the steps 
necessary to get the patch over the finish line. And if you are just 
doing a drive-by review pointing out all the mistakes, but not willing 
to +2 in the end then please state that up front as well. To be clear, 
I'm not against drive-by reviews where the reviewers never intend to +2 
in the end (as they can still help me improve my code!!), but I just 
would like them to be up front about this.


Cheers,
Adam

On 10/13/2017 10:56 AM, Thiago Macieira wrote:

On Friday, 13 October 2017 04:04:46 PDT Viktor Engelmann wrote:

  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.

I urge all reviewers to adopt the three-phase review process outlined in Sarah
Sharp's blog:

http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/

The only problem with this process is that different reviewers will reach
different stages at different times. So some of them may already have finished
the review of the concept and may be already providing criticism on the
implementation, while others are still not convinced of the concept.

Therefore, I add this extra advice on top of the blog:

When you're a reviewer in a change with multiple other reviewers, try to keep
pace with the other reviewers and not race ahead. If it's not evident where
the other reviewers are, communication is good. Post something like

"Thank you for your change, I like the idea and I think you implemented it
properly. I have some comments on your coding style, but we'll discuss that
when other reviewers have had a chance to review your implementation too."



___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Alexander Nassian]

I agree with all of your points except the one where you reject the fear model. 
Just a single very simple example: Try to download Qt.

You can buy the commercial license right from the home page, but in order to 
get the OSS version you have to click through numerous pages and have to search 
for the right links.

When you managed to get right before the download the user is confronted with 
numerous arguments against the OSS version of them some are very vague if not 
to say false.

Other example: I had a talk with several representatives who for example told 
me that „it‘s impossible to use Qt OSS in medical or automotive scenarios and I 
*have to* buy a license“. That‘s simply not true and shocked me even more.

I could also bring two examples from my personal contact to TQC but I don‘t 
think thats a thing for a public mailing list. If you are interested, we could 
talk about this in a private conversation.

These are just two examples from the last 1-2 years and there is always 
something new that encurages my view on the situation and it feels more and 
more like the OSS version is just a tumor on the feet of TQC.

My wishes would be that TQC makes either a clear statement that OSS is an equal 
option (and treat it like this), or be so honest and drop it. But this 
situation feels very embarassing to me.

And please don‘t get me wrong. I do respect your work at TQC, I do respect hat 
for you there must be a source of income, but there is are ways to achieve this 
without that bad habits that established the last 1-2 years.


Beste Grüße / Best regards,
Alexander Nassian

> Am 13.10.2017 um 15:15 schrieb Jake Petroules :
> 
> Thank you for addressing this point, Andy.
> 
> I also want to respond to Alex's other comment, "Let the community do the 
> work and squeeze all customers and force them to use commercial licensing by 
> using fear..."
> 
> First of all, look at the project statistics. The Qt Company is BY FAR the 
> largest contributor to the Qt Project.
> 
> Secondly, we don't use "fear" to "force" customers to use commercial 
> licensing. Each license grants a specific set of rights and privileges. If 
> you're not willing or unable to abide by the terms of the Open Source 
> licenses under which Qt is available, then you need to consider an 
> alternative which provides a different set of rights and privileges that may 
> be better suited for your use case, one of which is commercial licensing.
> 
> Also, please explain how The Qt Company will pay its expenses and its 
> employees, who in turn need to pay their rent and other bills, etc., without 
> a source of income. We're always looking for new ideas to expand our business 
> model, so if we could get free money out of thin air, that would be quite 
> excellent and we'd love for you to tell us how.
> 
>> On Oct 13, 2017, at 3:00 PM, Andy Shaw  wrote:
>> 
>> To make it clear, it is Viktor’s opinion here and he can state it but it 
>> does not mean that it is the representation of all of The Qt Company here. 
>> It was one point of a larger mail which he took the time to write because he 
>> wants to open a discussion about how we can improve the review process as a 
>> community. Ideas are naturally welcome and not all of them are going to be 
>> agreeable to everyone, and not all of them are going to be accepted moving 
>> forward either.
>> 
>> I personally don’t agree with the specific point made regarding where the 
>> contribution has come from as I believe that everyone should be treated 
>> equally when it comes to reviewing, just because you are or have been an 
>> employee of The Qt Company should not grant you any extra lenience when it 
>> comes to reviewing. As Eddie has already mentioned for his own sake, I also 
>> appreciate the feedback and the fact that someone is critically looking at 
>> my change. I may have been working with Qt for 17 years, but I still learn 
>> and I still make mistakes, my changes should be reviewed in exactly the same 
>> manner as anyone elses should be. I expect that and prefer it too.
>> 
>> Andy
>> 
>> Fra: Development  på 
>> vegne av Alexander Nassian 
>> Dato: fredag 13. oktober 2017 14.51
>> Til: Somers Andre 
>> Kopi: "development@qt-project.org" 
>> Emne: Re: [Development] Speeding up the review process (was: PostgreSQL 
>> cross compile for Pi)
>> 
>> That shows exactly the mindset of the TQC administration. Let the community 
>> do the work and squeeze all customers and force them to use commercial 
>> licensing by using fear...
>> 
>> 
>> Beste Grüße / Best regards,
>> Alexander Nassian
>> 
>> 
>> Am 13.10.2017 um 14:46 schrieb André Somers :
>> 
>> 
>> 
>> Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
>> 4. I don't think we need to be as paranoid towards contributions from our 
>> own 

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Thiago Macieira]

On Friday, 13 October 2017 06:13:16 PDT Viktor Engelmann wrote:
> > Anyone with approver rights should be aware of his powers and use them
> > carefully, no matter if he is employed at The Qt Company or an
> > external contributor.
> 
> Sure - but let's think about this in a different context: imagine
> someone applies at your company. You invite them to a job interview and
> have one of your engineers do a technical interview with them.

Since you didn't name which company this is, what you said applies to any 
company. By that logic, we should apply the rule to anyone who has gone 
through a technical interview, regardless of which company it is.

I don't think you want to start comparing who has the strictest technical 
interview.

Anyway, no, this is just completely impossible and not allowed by the Qt 
Project governance. We must treat everyone equally, based only on their 
contributions to the project, not on the company they work for.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Thiago Macieira]

On Friday, 13 October 2017 04:04:46 PDT Viktor Engelmann wrote:
>  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.

I urge all reviewers to adopt the three-phase review process outlined in Sarah 
Sharp's blog:

http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/

The only problem with this process is that different reviewers will reach 
different stages at different times. So some of them may already have finished 
the review of the concept and may be already providing criticism on the 
implementation, while others are still not convinced of the concept.

Therefore, I add this extra advice on top of the blog:

When you're a reviewer in a change with multiple other reviewers, try to keep 
pace with the other reviewers and not race ahead. If it's not evident where 
the other reviewers are, communication is good. Post something like

"Thank you for your change, I like the idea and I think you implemented it 
properly. I have some comments on your coding style, but we'll discuss that 
when other reviewers have had a chance to review your implementation too."

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Thiago Macieira]

On Friday, 13 October 2017 05:07:54 PDT Jedrzej Nowacki wrote:
> > 7. when you have a certain count of +1's from people who have
> > approver rights

>  - I'm afraid that as a result of 7 people will not give +1s anymore 
> Review as a process is good, it may be annoying but it is proven to increase
> quality of the code.

When you have enough +1, you can ask for a Maintainer's summary approval. The 
Maintainer is supposed to judge whether the people who have given those +1 are 
knowledgeable in general. 

Whenever I do that as Maintainer, I write that it's approved based on the 
certain number of other +1.

It's also one of the ways I usually get my own changes through.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Edward Welbourne]

Sérgio Martins (13 October 2017 16:05)
> Some users have been complaining about the review process and have
> rotting patches, so I welcome brainstorming around this. Let's see if
> we can conclude improvements!

Indeed - and the remedy for that is, painfully enough, that we, as
developers, need to spend more time doing review.

Change review has been a recommended practice in our industry for
several decades; in the last decade, I've seen it actually become a
*standard* practice, too, which is most welcome.  However, as an
industry, we're so used to it not being a standard practice that we
aren't yet used to allocating to it the time it actually needs.  We all
just want to "get on with real work" - by which developers tend to mean:
hacking on code, making our own changes.  For code review to realise its
full benefits, we have to learn to make time for it - to count it as
part of the "real work".

While refinements to process might help - as might a more modern version
of Gerrit - there is no cure for "rotting patches" other than that *we*
review those patches.  That takes time - and takes it away from the
other parts of our work.

Relaxing the rules is relatively incidental: giving time to this crucial
part of our work is the necessary part of solving the problem,

Eddy.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Sérgio Martins]

On Fri, Oct 13, 2017 at 2:48 PM, Viktor Engelmann
 wrote:
> I am thinking about the scenario when I read a 300 line commit and I am
> unsure about some of the lines. Say it removes one include and adds
> another include.
>
> If that commit comes from someone whom I talk to every day - someone
> whom I know to be very concerned about security and privacy - and
> someone I know is competent and has approver rights - I might ignore
> these 2 lines and assume that the compiler will fail on the CI in case
> the removed header was still needed.
>
> When the commit comes from someone whom I have never heard of - I will
> look into whether there is a symbol that will now be resolved
> differently - and if there is, I assume that this "differently" opens a
> backdoor.

In defense of Viktor:

With some better wording (and no per-company aspect), the Open
Governance model already provides for something like that, since it's
a meritocracy.
It's not outlandish if you spend more time reviewing and testing
patches from people with low merit than with high merit.

Moving on...

Some users have been complaining about the review process and have
rotting patches, so I welcome brainstorming around this. Let's see if
we can conclude improvements!


Regards,
Sergio Martins
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Jedrzej Nowacki]

If you do like that then you are doing it wrong. Review process is _not_ based 
on a name / company / sun activity. It is  based on the change content. Even 
best people do mistakes.

Cheers,
  Jędrek

On piątek, 13 października 2017 15:48:51 CEST Viktor Engelmann wrote:
> I am thinking about the scenario when I read a 300 line commit and I am
> unsure about some of the lines. Say it removes one include and adds
> another include.
> 
> If that commit comes from someone whom I talk to every day - someone
> whom I know to be very concerned about security and privacy - and
> someone I know is competent and has approver rights - I might ignore
> these 2 lines and assume that the compiler will fail on the CI in case
> the removed header was still needed.
> 
> When the commit comes from someone whom I have never heard of - I will
> look into whether there is a symbol that will now be resolved
> differently - and if there is, I assume that this "differently" opens a
> backdoor.
> 
> On 13.10.2017 14:52, Marc Mutz wrote:
> > On 2017-10-13 13:04, Viktor Engelmann wrote:
> >>  * I don't think we need to be as paranoid towards contributions
> >> from
> >> our own employees as we need to be towards external contributions.
> > 
> > I believe you got that the wrong way around :)
> > 
> > Thanks,
> > Marc


___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Viktor Engelmann]

I am thinking about the scenario when I read a 300 line commit and I am
unsure about some of the lines. Say it removes one include and adds
another include.

If that commit comes from someone whom I talk to every day - someone
whom I know to be very concerned about security and privacy - and
someone I know is competent and has approver rights - I might ignore
these 2 lines and assume that the compiler will fail on the CI in case
the removed header was still needed.

When the commit comes from someone whom I have never heard of - I will
look into whether there is a symbol that will now be resolved
differently - and if there is, I assume that this "differently" opens a
backdoor.


On 13.10.2017 14:52, Marc Mutz wrote:
> On 2017-10-13 13:04, Viktor Engelmann wrote:
>>  * I don't think we need to be as paranoid towards contributions
>> from
>> our own employees as we need to be towards external contributions.
>
> I believe you got that the wrong way around :)
>
> Thanks,
> Marc
>

-- 

Viktor Engelmann
Software Engineer

The Qt Company GmbH
Rudower Chaussee 13
D-12489 Berlin
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

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Christian Kandeler]

On Fri, 13 Oct 2017 15:13:16 +0200
Viktor Engelmann  wrote:

> >>  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.  
> > Anyone with approver rights should be aware of his powers and use them
> > carefully, no matter if he is employed at The Qt Company or an
> > external contributor.  
> 
> Sure - but let's think about this in a different context: imagine
> someone applies at your company. You invite them to a job interview and
> have one of your engineers do a technical interview with them.
> 
> Do you afterwards go to the applicant and ask them if they thought your
> engineer was competent and fire your engineer if the applicant says "no"?

This is an incredibly arrogant stance to take. No contributor is above others 
because of their employer.


Christian
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Viktor Engelmann]

On 13.10.2017 14:22, André Hartmann wrote:
> Hi Victor,
>
> just my 2 cent to one part:
>
>>  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.
> Anyone with approver rights should be aware of his powers and use them
> carefully, no matter if he is employed at The Qt Company or an
> external contributor.

Sure - but let's think about this in a different context: imagine
someone applies at your company. You invite them to a job interview and
have one of your engineers do a technical interview with them.

Do you afterwards go to the applicant and ask them if they thought your
engineer was competent and fire your engineer if the applicant says "no"?

> As it was stated on this mailing list some weeks ago: Only approve if
> you can take the blame on breaking something.

We shouldn't blame in the first place. We are all trying to make Qt better.
I only wish I could spend more time doing actual WORK on Qt.

> Best regards,
> André

-- 

Viktor Engelmann
Software Engineer

The Qt Company GmbH
Rudower Chaussee 13
D-12489 Berlin
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

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Alexander Nassian]

Sorry if I insulted somebody, that was not my intend. But the proposal alone 
fits too well into numerous things that happened the last months to the public 
presentation of Qt and TQC. Starting with the simple task of trying to download 
the OSS version of Qt which is the most complicated possible way currently and 
even if you managed to come to the right page, you get scared by all the bad 
things TQC writes would happen if someone uses the L-GPL version.

Best regards,
Alexander Nassian

> Am 13.10.2017 um 15:05 schrieb Jedrzej Nowacki :
> 
> Please, do not jump immediately to a conclusion. It was Viktor's proposal 
> which does not represent "TQC administration" whatever it is. Qt-project has 
> own rules and it is self-governmented. Just to be fair, you could also notice 
> my answer to the proposal:
>> I do not agree. An employer name does not matter.
> and I do work for The Qt Company.
> 
> Cheers,
>  Jędrek
> 
> On piątek, 13 października 2017 14:51:13 CEST Alexander Nassian wrote:
>> That shows exactly the mindset of the TQC administration. Let the community
>> do the work and squeeze all customers and force them to use commercial
>> licensing by using fear...
>> 
>> 
>> Beste Grüße / Best regards,
>> Alexander Nassian
>> 
>>> Am 13.10.2017 um 14:46 schrieb André Somers :
>>> 
>>> Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
 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.> 
>>> So much for open government...
>>> 
>>> André
>>> 
>>> ___
>>> Development mailing list
>>> Development@qt-project.org
>>> http://lists.qt-project.org/mailman/listinfo/development
> 
> 


-- 
 


—

bitshift dynamics GmbH
Neudorfer Str. 1, 79541 Lörrach
Registergericht: Amtsgericht Freiburg i. Breisgau, HRB 713747
Geschäftsführer: Alexander Nassian, Markus Pfaffinger

http://www.bitshift-dynamics.de

Zentrale: +49 762158673 - 0
Fax: +49 7621 58673 - 90

Allgemeine Anfragen: i...@bitshift-dynamics.com
Technischer Support: supp...@bitshift-dynamics.com
Buchhaltung: invo...@bitshift-dynamics.com
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Jedrzej Nowacki]

Please, do not jump immediately to a conclusion. It was Viktor's proposal 
which does not represent "TQC administration" whatever it is. Qt-project has 
own rules and it is self-governmented. Just to be fair, you could also notice 
my answer to the proposal:
> I do not agree. An employer name does not matter.
and I do work for The Qt Company.

Cheers,
  Jędrek

On piątek, 13 października 2017 14:51:13 CEST Alexander Nassian wrote:
> That shows exactly the mindset of the TQC administration. Let the community
> do the work and squeeze all customers and force them to use commercial
> licensing by using fear...
> 
> 
> Beste Grüße / Best regards,
> Alexander Nassian
> 
> > Am 13.10.2017 um 14:46 schrieb André Somers :
> > 
> > Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
> >> 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.> 
> > So much for open government...
> > 
> > André
> > 
> > ___
> > Development mailing list
> > Development@qt-project.org
> > http://lists.qt-project.org/mailman/listinfo/development


___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Andy Shaw]

To make it clear, it is Viktor’s opinion here and he can state it but it does 
not mean that it is the representation of all of The Qt Company here. It was 
one point of a larger mail which he took the time to write because he wants to 
open a discussion about how we can improve the review process as a community. 
Ideas are naturally welcome and not all of them are going to be agreeable to 
everyone, and not all of them are going to be accepted moving forward either.

I personally don’t agree with the specific point made regarding where the 
contribution has come from as I believe that everyone should be treated equally 
when it comes to reviewing, just because you are or have been an employee of 
The Qt Company should not grant you any extra lenience when it comes to 
reviewing. As Eddie has already mentioned for his own sake, I also appreciate 
the feedback and the fact that someone is critically looking at my change. I 
may have been working with Qt for 17 years, but I still learn and I still make 
mistakes, my changes should be reviewed in exactly the same manner as anyone 
elses should be. I expect that and prefer it too.

Andy

Fra: Development  på vegne 
av Alexander Nassian 
Dato: fredag 13. oktober 2017 14.51
Til: Somers Andre 
Kopi: "development@qt-project.org" 
Emne: Re: [Development] Speeding up the review process (was: PostgreSQL cross 
compile for Pi)

That shows exactly the mindset of the TQC administration. Let the community do 
the work and squeeze all customers and force them to use commercial licensing 
by using fear...


Beste Grüße / Best regards,
Alexander Nassian


Am 13.10.2017 um 14:46 schrieb André Somers 
>:



Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
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.

So much for open government...

André
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development




—

bitshift dynamics GmbH
Neudorfer Str. 1, 79541 Lörrach
Registergericht: Amtsgericht Freiburg i. Breisgau, HRB 713747
Geschäftsführer: Alexander Nassian, Markus Pfaffinger

http://www.bitshift-dynamics.de

Zentrale: +49 762158673 - 0
Fax: +49 7621 58673 - 90

Allgemeine Anfragen: 
i...@bitshift-dynamics.com
Technischer Support: 
supp...@bitshift-dynamics.com
Buchhaltung: invo...@bitshift-dynamics.com
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Marc Mutz]

On 2017-10-13 13:04, Viktor Engelmann wrote:

* I don't think we need to be as paranoid towards contributions from
our own employees as we need to be towards external contributions.


I believe you got that the wrong way around :)

Thanks,
Marc

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Edward Welbourne]

Viktor Engelmann (13 October 2017 13:04)
> 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.

I have one simple suggestion: if you find code review is holding up your
work, go and review some other folks' changes to code.  Assume, unless
proven otherwise, that you need to spend as much time reviewing others'
code as you spend waiting for review.

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

I do not want this privilege.
I make dumb mistakes sometimes.
I rely on reviewers to smack me upside the head and get me to fix them.
I routinely notice people I know are smart making similar mistakes; and
they're usually grateful for having them politely brought to their
attention, as I am with mine.

Eddy.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Alexander Nassian]

That shows exactly the mindset of the TQC administration. Let the community do 
the work and squeeze all customers and force them to use commercial licensing 
by using fear...


Beste Grüße / Best regards,
Alexander Nassian

> Am 13.10.2017 um 14:46 schrieb André Somers :
> 
> 
> 
> Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
>> 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.
>> 
> So much for open government...
> 
> André
> 
> ___
> Development mailing list
> Development@qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development


-- 
 


—

bitshift dynamics GmbH
Neudorfer Str. 1, 79541 Lörrach
Registergericht: Amtsgericht Freiburg i. Breisgau, HRB 713747
Geschäftsführer: Alexander Nassian, Markus Pfaffinger

http://www.bitshift-dynamics.de

Zentrale: +49 762158673 - 0
Fax: +49 7621 58673 - 90

Allgemeine Anfragen: i...@bitshift-dynamics.com
Technischer Support: supp...@bitshift-dynamics.com
Buchhaltung: invo...@bitshift-dynamics.com
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[André Somers]

Op 13/10/2017 om 13:04 schreef Viktor Engelmann:
> 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.
>
>
So much for open government...

André

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[André Hartmann]

Hi Victor,

just my 2 cent to one part:


 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.
Anyone with approver rights should be aware of his powers and use them 
carefully, no matter if he is employed at The Qt Company or an external 
contributor.


As it was stated on this mailing list some weeks ago: Only approve if 
you can take the blame on breaking something.


Best regards,
André
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Jedrzej Nowacki]

On piątek, 13 października 2017 13:04:46 CEST Viktor Engelmann wrote:
> 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:
>  1. when you only changed minor things like a variable name (except
> in public API), a string, a comment or qdoc entry
>  2. 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 )
>  3. 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.
>  4. or more general: when you only made an autotest harder to pass -
> like adding a Q_VERIFY, Q_COMPARE, etc.
>  5. when the change is something auto-generated - like you just
> updated plugins.qmltypes using qmlplugindump
>  6. when you only changed something in accordance to the guidelines
> - like Q_DECL_OVERRIDE -> override
>  7. when you have a certain count of +1's from people who have
> approver rights
That doesn't solve the problem, no brainers are getting review quite quickly. 
Some more detailed comments below:
 - In 2, you would need to amend the commit message too.
 - 3 is  at best controversial, do not increase timeouts, rewrite the test 
instead.
 - I'm not convinced with 4, it may happen that one is enforcing wrong or 
invalid behaviour.
 - I'm afraid that as a result of 7 people will not give +1s anymore :-)
Review as a process is good, it may be annoying but it is proven to increase 
quality of the code.

>  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!
I like how you believe in CI, I share it, but check our test coverage, we are 
far from perfect.

>  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.
The sentence suggests that we have code review process to cut CI expenses, 
that is simply not true. Anyway, it is not that easy as you wrote there are 
limits of scalability. 

>  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.
I do not agree. An employer name does not matter.

>  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.
I agree that such situation should not happen, it really should be avoided. I 
have experienced that few times and I know how annoying and demotivating it 
is. In the same time it can not be an excuse to commit a broken code.


I do agree that it takes to long to pass code review, but I do not think that 
giving up on review is a good idea.

Cheers,
  Jędrek
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Konstantin Tokarev]

13.10.2017, 14:05, "Viktor Engelmann" :
> 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
>
> * 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 )

FWIW, in WebKit project it's a usual workflow to set "r+" and at the same time 
request minor changes to the patch, fixed version of patch is submitted without 
new review.

> * 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

Note that at least some of this rules can be implemented in gerrit itself, so 
you don't need self-approve

https://codereview.qt-project.org/Documentation/prolog-cookbook.html
https://codereview.qt-project.org/Documentation/prolog-change-facts.html

> * 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!
> * 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.
> * I don't think we need to be as paranoid towards contributions from our own 
> employees as we need to be towards external contributions.

Hey, it's a discrimination!

> * 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 +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
> ,
>
> ___
> 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