Re: [QGIS-Developer] QGIS repository management
On Tue, 17 Oct 2023 at 03:41, Sandro Santilli 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 wouldn't trust myself not to make accidental mistakes, or do something inefficient, or that I'm not duplicating code which is present elsewhere in QGIS. A second set of eyes over a pull request definitely reduces that risk. QGIS is MASSIVE, and none of us can keep a complete picture of the impact of code changes in our heads. Some real world examples, picked at semi-random (because they are recent): https://github.com/qgis/QGIS/pull/54941 : Submitted by a core developer. I reviewed, and it looked good to me. But then a third (!) set of eyes over the code has revealed a massive potential performance regression caused by the change (thanks Even!). https://github.com/qgis/QGIS/pull/54670: Submitted by a very experienced core developer. Code looks good at first pass, no visible bugs. But it's introducing a partial duplication of another utility function implemented elsewhere and exhaustively tested. The review identified that and as a result we avoided introducing an unnecessary, non trivial duplication of code. That's two examples. I'd very conservatively estimate that 1 in 10 approved PRs submitted by core committers have at least one set of changes suggested and implemented. Let's (pessimistically!) write half of those off as non-bugs, such as optimisation suggestions or changes relating to documentation/comments/code style. That's still **AT BEST** 1 in 20 pull requests submitted by core committers which needed fixes. 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.. Nyall > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. > > --strk; ___ 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
Re: [QGIS-Developer] QGIS repository management
[Intranet logo] I'm far from a regular contributor to QGIS and thus not affected by this issue, but I see this review process the same way I see how peer-reviewed research papers get published (but a bit less stringent). In research, your manuscript is sent to 3 or 4 reviewers that will give comments, then you make corrections and resend the manuscript for a final review before getting published. This workflow is valid for every researcher that want to publish in a peer-reviewed journal, even for those that are also reviewer. It can be slow and frustrating at times, but having an other set of eyes on your work usually result in a better overall paper. Software development is different than peer-reviewed research publication but getting inspiration by this centuries old process is certainly appropriate. Going back to QGIS, I see the value in having the PR of a reviewer reviewed by an other reviewer in this second fresh set of eyes that can improve on an already fine job. Every reviewer can be trusted for their expertise and judgement... but needing someone else to review your code shouldn't be seen as distrustful. As for the conflict of interest if the reviewer works for the same company, in the academia world, the reviewer would be in conflict of interest (real or not) and would need to withdraw (the equivalent of a company being the same department of a research institute or a close collaborator). Considering that in academia this constrain has a lot to do with avoiding an idea being pushed without criticism, it might be overkill in software development where the ideas/orientations are not decided on a PR basis, but more at the PSC and with the general consensus between the devs (that's how I see it, I might be wrong). Jean-François Bourdon, ing.f. Analyste en télédétection Direction des inventaires forestiers Ministère des Ressources naturelles et des Forêts 5700, 4e Avenue Ouest, local A-108 Québec (Québec) G1H 6R1 Téléphone : 418 627-8669, poste 704304 jean-francois.bour...@mrnf.gouv.qc.ca mrnf.gouv.qc.ca -Message d'origine- De : QGIS-Developer De la part de Sandro Santilli via QGIS-Developer Envoyé : 16 octobre 2023 10:57 À : Nyall Dawson ; qgis-developer@lists.osgeo.org Objet : Re: [QGIS-Developer] QGIS repository management On Mon, Oct 16, 2023 at 04:41:42PM +0200, Sandro Santilli via QGIS-Developer 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 ! > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. My suggestion here is that having given someone "write privileges" status means the community trusts that individual to know the rules and be able to apply them, in a way that does not striclty requires someone else to guard after his/her work. This isn't to say that reviews aren't important and that all of use should always aim at having reviews, but that the judgement abilities of the "writers" should be trusted, and if that trust is not hold anymore the "write privileges" should be revoked, following a documented procedure. --strk; ___ 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
Re: [QGIS-Developer] QGIS repository management
On Mon, Oct 16, 2023 at 04:41:42PM +0200, Sandro Santilli via QGIS-Developer 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 ! > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. My suggestion here is that having given someone "write privileges" status means the community trusts that individual to know the rules and be able to apply them, in a way that does not striclty requires someone else to guard after his/her work. This isn't to say that reviews aren't important and that all of use should always aim at having reviews, but that the judgement abilities of the "writers" should be trusted, and if that trust is not hold anymore the "write privileges" should be revoked, following a documented procedure. --strk; ___ 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
Re: [QGIS-Developer] QGIS repository management
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 ! Beside the same-company reviews things, consider I could always ask a friend to file PRs for the sole purpose of me being able to merge them in (I cannot merge my own...). The policy is flawed to me. --strk; ___ 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
Re: [QGIS-Developer] Dropping "render" checkbox from status bar
Hi Nyall / all, You might be interested that the Ordnance Survey documentation for OS Features API makes use of the "render" check box in the setup when pulling data from their API. See https://osdatahub.os.uk/docs/wfs/gettingStarted under QGIS and search for "render". Whether or not this is a good idea or is needed I don't know but I thought you might be interested that it is used here. For the record - I am not linked with Ordnance Survey and didn't write the instructions, I just used them a couple of times! Best wishes, Nick. On 16/10/2023 07:47, Régis Haubourg via QGIS-Developer wrote: Same here. I use this when playing with huge datasets. Cheers Régis Le dim. 15 oct. 2023, 23:39, Nicolas Cadieux via QGIS-Developer a écrit : Hi, Still useful on my side when rendering very heavy stuff and then decide to zoom to another part of the map… Nicolas Cadieux > Le 15 oct. 2023 à 15:30, Nyall Dawson via QGIS-Developer a écrit : > > Hi list, > > Question: are there still any valid use cases for the "render" > checkbox in the status bar? It's been around forever, and DID have a > strong use case when map rendering used to block the QGIS interface. > But is there any reason to keep this around in modern QGIS versions? > > Nyall > ___ > 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 ___ 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 -- Nick Bearman +44 (0) 7717745715 n...@geospatialtrainingsolutions.co.uk Due to my own life/work balance, you may get emails from me outside of normal working hours. Please do not feel any pressure to respond outside of your own working pattern. ___ 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
Re: [QGIS-Developer] QGIS repository management
As someone on the outside of the committer set (which is fine!) for qgis, but familiar with the larger issues (and a packager): I'm going to use the word committer. git has renamed things, but the point is of course placing state in the branch that matters in the repo that matters. There's a question about how the rules evolved, and how they change, and that's hard to answer, but it's not really a big issue. The rules not being written down and understandable is a big issue with an easy fix. It is quite easy for someone who knows them to just put them in a HACKING.md or CONTRIBUTING.md if you lean to github vs GNU. Sandro's point about 1 committer approving a stranger's change has some merit, but I think we have to separate: - review to avoid bugs - review to avoid injection of malware Presumably, one would only approve/merge a chagne if it is clean, neat, meets standards, works, etc., and if it were more complicated a committer should ask others/wait/etc. If we're going to insist on CI passing to merge, then it really needs to pass essentially all the time. This means disabling parts of it if they are flaky, or demoting them to a 2nd workflow that is not considered a failure. ___ 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
Re: [QGIS-Developer] Dropping "render" checkbox from status bar
Same here. I use this when playing with huge datasets. Cheers Régis Le dim. 15 oct. 2023, 23:39, Nicolas Cadieux via QGIS-Developer < qgis-developer@lists.osgeo.org> a écrit : > Hi, > > Still useful on my side when rendering very heavy stuff and then decide to > zoom to another part of the map… > > Nicolas Cadieux > > > Le 15 oct. 2023 à 15:30, Nyall Dawson via QGIS-Developer < > qgis-developer@lists.osgeo.org> a écrit : > > > > Hi list, > > > > Question: are there still any valid use cases for the "render" > > checkbox in the status bar? It's been around forever, and DID have a > > strong use case when map rendering used to block the QGIS interface. > > But is there any reason to keep this around in modern QGIS versions? > > > > Nyall > > ___ > > 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 > ___ 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
