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