Hi,
Even if it's sometime painful to wait for a review, I completely suscribe to the actual processus of review by another peer for the reasons well explained by others. IMHO, the main issue here is that the CI is too often broken for no reasons related to the PR content. And most of the time recently, it's because of one of the mingw jobs. Like proposed by Matthias, I would be very much in favor in modifying our CI jobs to rely on non-rolling release distribution. Regards, Julien > In my experience, the peer reviews have proven to be an effective tool to > improve the code quality. > I think it can be explained with a four eyes principle. The first two eyes > are involved in writing the code, the second pair of eyes that validates > needs to be a trusted pair. > For a code base of the maturity of QGIS, this seems to me to be justified and > reasonable. > > As for who can do a review, a potential conflict of interest has been raised. > This arises most noticeable within a single company, but can also > very well be the case whenever a collaboration occurs between multiple > individuals or if a well-connected client is involved. In my experience, in > most cases people will first ask the person that they trust to be most > qualified to review a specific area of the code, regardless the organisation > they work for. In some cases, this may be your colleagues. What I and others > have done in the past in such cases, is to leave an additional review > window of a couple of days before merging to give everyone the opportunity to > jump in if they wanted to, just to be sure to play it fair. In my > experience, this worked quite nicely, but I am also open to other approaches. > > As for failing tests, I think one possibly low hanging fruit would be to > switch all CI workflows to be based on non-rolling distributions. And we > could potentially also be a bit more strict with disabling flaky tests or > making them optional. > > Best wishes > Matthias > > On Tue, Oct 17, 2023 at 9:47 AM Sandro Santilli via QGIS-Developer > <qgis-developer@lists.osgeo.org> wrote: > > On Tue, Oct 17, 2023 at 03:08:34PM +1300, Nyall Dawson via QGIS-Developer > wrote: > > On Tue, 17 Oct 2023 at 03:41, Sandro Santilli <s...@kbt.io> wrote: > > > > > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via > QGIS-Developer wrote: > > > > > > > If you flip the situation, you'll see that yes, you do have trust! > > > > > > > > - a complete stranger CANNOT approve their own changes > > > > - a complete stranger CANNOT approve other stranger's changes > > > > - a complete stranger CANNOT approve an approved member's changes > > > > > > > > vs > > > > > > > > - an approved member CANNOT approve their own changes > > > > - an approved member CAN approve a complete stranger's changes > > > > - an approved member CAN approve a another approved member's changes > > > > > > That's partial trust. I'm trusted to be able to judge someone else's > > > work but not to judge my own work ! > > > > Ok, it's partial trust. What's wrong with that? > > I feel I'm not explaining myself correctly. > I do believe in peer review, and I'm happy for others to look at the > changes I propose. I'd love everyone to look at them! > > The word "partial" was not appropriate, probably, it's more about > "inconsistency". > > > In short: I'm more than OK with only partial trusting everyone. We ALL > > make mistakes, so let's stick with the processes we have which reduce > > the risk of these mistakes hitting end users.. > > I know I make mistakes, but I'm able to merge changes proposed by a > random contributor, where my own review is enough. > > I'm FULLY trusted in that case, and this is conflicting with not > being so when it's me proposing the changes. > > Why should I be able to merge in that case then ? Is it because > 2 people review the code ? Me and the proponent ? Then why the > random contributor review would not count on MY prs ? > > I hope you see the discrepancy here. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html > _______________________________________________ > QGIS-Developer mailing list > QGIS-Developer@lists.osgeo.org > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer > > _______________________________________________ > QGIS-Developer mailing list > QGIS-Developer@lists.osgeo.org > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer -- Julien Cabieces Senior Developer at Oslandia julien.cabie...@oslandia.com _______________________________________________ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
