Glad to see such positive feedback :) . To be honest - I've been interacting with ~ 120 PR over the last few months and for me it was only positive. I had a few cases where I wanted to merge a PR and got "1 conversation unresolved" and it made me come back:
Particularly this one https://github.com/apache/airflow/pull/36513 - with 68 (!) conversations where I got a lot of comments from Bas and I wanted to make sure I address all of them (and Github's interface hiding conversations if there are many) does not make it easy. There were quite a few I'd have missed if not the "2 unresolved conversations". I also had an exchange with Bolke on https://github.com/apache/airflow/pull/22253 - which was a very old PR, with past conversations, where I personally think having to come back and re-review the conversations that were not resolved from long past are - I believe at least - great refresher when you come back to a PR from long past. Definitely +1 from me. J. On Tue, Jan 30, 2024 at 7:43 PM Oliveira, Niko <oniko...@amazon.com.invalid> wrote: > Yeah, I've found this to be pretty smooth as well. In most cases comments > were already resolved and in lesser cases it was useful to see which > conversations still needed addressing before merging. +1 from me! > > ________________________________ > From: Ryan Hatter <ryan.hat...@astronomer.io.INVALID> > Sent: Tuesday, January 30, 2024 8:58:30 AM > To: dev@airflow.apache.org > Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [ANNOUNCE] Starting > experimenting with "Require conversation resolution" setting > > CAUTION: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. > Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez > pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que > le contenu ne présente aucun risque. > > > > In my experience outside of Airflow, the benefit of not missing a review > comment outweighs the friction of being required to resolve each > conversation. > > On Mon, Jan 29, 2024 at 8:47 PM Wei Lee <weilee...@gmail.com> wrote: > > > I didn't notice much of a difference as a contributor. +1 vote > > > > Best, > > Wei > > > > > On Jan 30, 2024, at 11:41 AM, Amogh Desai <amoghdesai....@gmail.com> > > wrote: > > > > > > Contrary to my initial expectation of the trouble this would bring in > for > > > reviewers, it has been > > > pretty nice. I have not faced any issues in marking the conversations > as > > > resolved for the pull > > > requests I have reviewed and it has even given me a chance to re review > > > prior to approval. > > > > > > I am happy with this overall and my vote will be a +1 > > > > > > Thanks & Regards, > > > Amogh Desai > > > > > > On Mon, Jan 29, 2024 at 7:56 PM Aritra Basu <aritrabasu1...@gmail.com> > > > wrote: > > > > > >> I personally haven't had too much friction due to the change and it > has > > >> helped me keep track of any comments people have made. I remain +1 to > > the > > >> change so far. > > >> > > >> -- > > >> Regards, > > >> Aritra Basu > > >> > > >> On Mon, Jan 29, 2024, 6:11 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > >> > > >>> Just wanted to remind everyone, we are nearing the end of the trial > > >> period > > >>> for "require conversation" feature to be enabled. I have my own > > >>> observations and examples, but since I was the one to propose it, I > am > > >>> likely biased, so I'd love to hear from others what their feedback > and > > >>> assessment is. Or maybe we need more time to assess it ? > > >>> > > >>> I would love to hear your thoughts. > > >>> > > >>> J, > > >>> > > >>> > > >>> On Sat, Dec 30, 2023 at 2:20 PM Jarek Potiuk <ja...@potiuk.com> > wrote: > > >>> > > >>>> After an initial indentation problem in .asf.yaml it's not working > as > > >>>> expected. So .... let's see how resolving conversations will work > for > > >> us. > > >>>> > > >>>> On Sat, Dec 30, 2023 at 12:17 PM Amogh Desai < > > amoghdesai....@gmail.com > > >>> > > >>>> wrote: > > >>>> > > >>>>> Wooho! Looking to see how this turns out for airflow 😃 > > >>>>> > > >>>>> On Sat, 30 Dec 2023 at 1:35 PM, Jarek Potiuk <ja...@potiuk.com> > > >> wrote: > > >>>>> > > >>>>>> Hello everyone, > > >>>>>> > > >>>>>> As discussed in > > >>>>>> https://lists.apache.org/thread/cs6mcvpn2lk9w2p4oz43t20z3fg5nl7l > I > > >>> just > > >>>>>> enabled "require conversation resolution" for our main/stable > > >>> branches. > > >>>>> We > > >>>>>> have not used it in the past so it might not work as we think or > we > > >>>>> might > > >>>>>> need to tweak something. > > >>>>>> > > >>>>>> Generally speaking (if all works) all conversations on PRs should > be > > >>>>>> resolved before we can merge the PR. This "resolving" is > encouraged > > >> to > > >>>>> be > > >>>>>> done by the author when they think the conversation is resolved, > but > > >>> it > > >>>>> can > > >>>>>> also be done by reviewers or the maintainer who wants to merge the > > >> PR. > > >>>>>> > > >>>>>> We attempted to describe some basic rules and expectations here: > > >>>>>> > > >>>>>> > > >>>>> > > >>> > > >> > > > https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#step-5-pass-pr-review > > >>>>>> but undoubtedly there will be questions and issues that we might > > >> want > > >>> to > > >>>>>> solve - so feel free to discuss it here or raise question/issues > in > > >>>>>> #development channel in slack (I am also happy to be pinged > directly > > >>>>> about > > >>>>>> it and help to resolve any issues/gather feedback). > > >>>>>> > > >>>>>> J. > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > >
