So while I have not experienced real challenges with it except for a PR that was really old and the github ui was hiding the comments, I have also not experienced any benefits. Sometimes resolving comments might als *hide* the actual thread. That means I can resolve without resolving and push things through because others won't see the discussion.
So the value added to me is not there and therefore I consider it an administrative burden. So -1 or 0 to this. Bolke. On Tue, 30 Jan 2024 at 21:32, Hussein Awala <huss...@awala.fr> wrote: > +1 for keeping it, it's useful to avoid forgetting old conversations, even > if they are sometimes outdated (ex #22253), spending 2 minutes closing them > is better than merging the PR without considering some of them. > > On Tue, Jan 30, 2024 at 8:11 PM Ferruzzi, Dennis > <ferru...@amazon.com.invalid> wrote: > > > So far, my only real down side is that sometimes discussions happen in > > comments (not in threaded code comments, but in one-off "add a comment" > > comments) which this doesn't capture and they are annoying to track. > While > > that isn't an actual issue with this CI change, this change may possibly > > lead to complacency on making sure THOSE are all resolved as well. Not > > worth a -1, just an honest thought and a reminder to all of us (myself > > included) that we still need to check if those are resolved. > > > > > > - ferruzzi > > > > > > ________________________________ > > From: Jarek Potiuk <ja...@potiuk.com> > > Sent: Tuesday, January 30, 2024 10:55 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. > > > > > > > > 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 > > > > > > > > > > > > > > -- -- Bolke de Bruin bdbr...@gmail.com
