> Given the slow down over the holidays I don't think two weeks will be enough - make it 4?
Ah. True :) On Tue, Dec 19, 2023 at 9:41 PM Ash Berlin-Taylor <a...@apache.org> wrote: > Given the slow down over the holidays I don't think two weeks will be > enough - make it 4? > > On 19 December 2023 20:33:23 GMT, Jarek Potiuk <ja...@potiuk.com> wrote: > >Ash: > > > >> I.e. Convention over enforcement and treating people as mature adults > not > >children who need guard rails. > > > >I think it's quite the opposite, Both 1) 2) and 3) reasoning is more of > the > >aid for whoever looks at the PR that there are still some conversations > not > >addressed, I personally feel it's treating people as more adult, when you > >allow them to unilaterally say "I believe the conversation is resolved" > > > >Bolke: > > > >> This reflect my feelings as well. I'm not convinced we are solving > >something that needs to be solved. > > > >I think 1) 2) 3) are real problems that it addresses. > > > > > >If we have no "strong" -1s we can give it a try. It's not a one-way > street. > >We can always go back if we see it slows us down or annoys people. > > > >We can even set a way how we assess it. Maybe everyone should just collect > >cases where it caused some problems - in two weeks or so (should be > enough). > >Why not everyone who actively participates in the PR review process brings > >their experience and explains if it caused them unnecessary burden for no > >gain (also the opposite - where it helped). > > > >J. > > > > > >On Tue, Dec 19, 2023 at 9:15 PM Jarek Potiuk <ja...@potiuk.com> wrote: > > > >> Answering some of the recent questions. > >> > >> Daniel: > >> > >> > E.g. when you "resolve" a conversation, then you make it less > visible. > >> This isn't always a good thing. Sometimes you just want to +1 it. When > >> others visit the PR, then they will not see the conversation. Maybe they > >> would want to engage in discussion. And when you get a notification in > an > >> email about a comment and want to engage and respond, I've had issues > with > >> following links to conversations after resolution before. > >> > >> True. However, you still see that there was conversation (and can always > >> un-collapse it). Resolving conversation does not "remove" it. Actually > when > >> the conversation is resolved. > >> Also you can see it in the "conversations menu". And well, assumption is > >> that resolving conversation makes it well - resolved :). And +1 until > it is > >> resolved is still fine and cool (or even after). And as a maintainer, > you > >> can always "unresolve" such a conversation. > >> > >> [image: image.png] > >> > >> Hussein: > >> > >> > The proposed rule isn't a bad idea, especially for ensuring that > >> maintainers wanting to merge have reviewed all conversions. However, > it's > >> essential to permit them to close open conversations if they find the > >> comments have been addressed. Only ping the commenter if uncertain, > with a > >> maximum waiting time (let's say 48 hours on workdays). If the commenter > >> doesn't reply and there are no other open conversations, we can merge. > >> > >> Absolutely. I think it should be fine to have either the author or the > >> maintainer to resolve conversations - it all depends on context and > >> problem. I tend to think about "resolving" conversation as a statement > of > >> intention / understanding rather than "certainty". It might be either > the > >> author or the maintainer who "BELIEVES" that the conversation is > resolved. > >> It's subjective, not objective IMHO. We - as humans - can make mistakes > but > >> as long as we have good intentions, it's fine to resolve conversation by > >> either. What matters here is that no conversation is simply "left > >> unaddressed" and that there was a deliberate action on each > conversation. > >> > >> From > >> > https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations > >> > >> > You can resolve a conversation in a pull request if you opened the > pull > >> request or if you have write access to the repository where the pull > >> request was opened. > >> > >> So in our case - either the author, or one of the maintainers can mark > the > >> conversation as resolved. > >> > >> > Could we forbid the authors from closing conversations? > >> > >> I am afraid not. > >> > >> > >> On Tue, Dec 19, 2023 at 7:31 PM Hussein Awala <huss...@awala.fr> wrote: > >> > >>> The proposed rule isn't a bad idea, especially for ensuring that > >>> maintainers wanting to merge have reviewed all conversions. However, > it's > >>> essential to permit them to close open conversations if they find the > >>> comments have been addressed. Only ping the commenter if uncertain, > with a > >>> maximum waiting time (let's say 48 hours on workdays). If the commenter > >>> doesn't reply and there are no other open conversations, we can merge. > >>> Additionally, should we wait for all open conversations or only those > >>> opened by maintainers? Could we forbid the authors from closing > >>> conversations? > >>> > >>> On Tue 19 Dec 2023 at 19:19, Daniel Standish > >>> <daniel.stand...@astronomer.io.invalid> wrote: > >>> > >>> > +1 > >>> > > >>> > On Tue, Dec 19, 2023 at 9:36 AM Pierre Jeambrun < > pierrejb...@gmail.com> > >>> > wrote: > >>> > > >>> > > This is something I already try to apply on my own PRs, never merge > >>> > before > >>> > > explicitly solving all conversations. > >>> > > > >>> > > Also for a reviewer, I feel like this gives more confidence to the > >>> fact > >>> > > that the PR is ready, and indeed we are less subject to missing a > >>> > > discussion or something going on making it 'not ok' to merge. Going > >>> over > >>> > > the entire thread before merging a PR to double check that > everything > >>> is > >>> > > actually addressed can be time consuming. That is especially true > if > >>> > things > >>> > > are not marked as resolved. > >>> > > > >>> > > I agree that this is something that adds up some work, but I think > it > >>> is > >>> > > worth the experiment and see what happens. We can easily revert if > we > >>> are > >>> > > not happy with the way it goes. > >>> > > > >>> > > The workload will most likely be on the contributors' side, that > will > >>> > have > >>> > > to actually address and solve all the conversations. > >>> > > > >>> > > Le mar. 19 déc. 2023 à 16:44, Vincent Beck <vincb...@apache.org> a > >>> > écrit : > >>> > > > >>> > > > I am wondering too if this is not something that gives more work > to > >>> > > > maintainer without real benefits. A maintainer can still mark all > >>> > > > conversations as resolved and merge the PR if he wants. Though, I > >>> > > > understand there is the intention here as oppose as today where a > >>> > > > maintainer can just miss some comments. I am quite doubtful but I > >>> am in > >>> > > to > >>> > > > try it out and see how it goes. > >>> > > > > >>> > > > On 2023/12/19 14:55:13 Bolke de Bruin wrote: > >>> > > > > I'm less enthusiastic. What problem are we solving with this? > If > >>> > > > something has not been addressed it can be done in a follow up or > >>> of if > >>> > > it > >>> > > > was just part of the conversation it won't have impact on the > code. > >>> In > >>> > > > addition, the ones that need to deal with it are the ones merging > >>> and > >>> > > those > >>> > > > are not necessarily the same as the ones contributing. > >>> > > > > > >>> > > > > So for the friction that it creates with both the committer and > >>> the > >>> > > > contributer what is the benefit? > >>> > > > > > >>> > > > > B. > >>> > > > > > >>> > > > > > >>> > > > > Sent from my iPhone > >>> > > > > > >>> > > > > > On 19 Dec 2023, at 15:45, Wei Lee <weilee...@gmail.com> > wrote: > >>> > > > > > > >>> > > > > > +1 for trying and observing how it works. My concern is that > >>> > adding > >>> > > > an additional obstacle might lead to more unfinished PRs. It > might > >>> be > >>> > > > helpful to give the contributor some guidance on when we can > resolve > >>> > the > >>> > > > comments. > >>> > > > > > > >>> > > > > > Best, > >>> > > > > > Wei > >>> > > > > > > >>> > > > > >> On Dec 19, 2023, at 9:28 PM, Andrey Anshin < > >>> > > andrey.ans...@taragol.is> > >>> > > > wrote: > >>> > > > > >> > >>> > > > > >> We could try and if found it slows down for some reason we > >>> always > >>> > > > might > >>> > > > > >> revert it back. > >>> > > > > >> > >>> > > > > >> Just one suggestion, sometimes discussion contains some > useful > >>> > > > information, > >>> > > > > >> e.g. "What the reason of finally decision", "Useful > information > >>> > why > >>> > > it > >>> > > > > >> should works by suggested way, or should not work", which > >>> might be > >>> > > > useful > >>> > > > > >> for someone who investigate why this changes was made, so in > >>> this > >>> > > > case I > >>> > > > > >> would suggest to create link in the main thread of PR with > >>> useful > >>> > > > > >> discussions. > >>> > > > > >> > >>> > > > > >> > >>> > > > > >> > >>> > > > > >> > >>> > > > > >>> On Tue, 19 Dec 2023 at 17:16, Jarek Potiuk < > ja...@potiuk.com> > >>> > > wrote: > >>> > > > > >>> > >>> > > > > >>> Hey everyone, > >>> > > > > >>> > >>> > > > > >>> TL;DR; I have a small proposal/discussion proposal to > modify a > >>> > bit > >>> > > > the > >>> > > > > >>> branch protection rules for Airflow. Why don't we add a > >>> > protection > >>> > > > > >>> rule in our PRs that requires all the comments in the PRs > to > >>> be > >>> > > > > >>> "marked as resolved" before merging the PR ? > >>> > > > > >>> > >>> > > > > >>> I have been following myself - for quite some time - an > >>> approach > >>> > > > that > >>> > > > > >>> whenever there are comments/suggestions/doubts in my PRs I > do > >>> not > >>> > > > > >>> merge the PR until I **think** all of those have been > >>> addressed > >>> > > > > >>> (somehow). > >>> > > > > >>> > >>> > > > > >>> The resolution might not be what the person commenting > wants > >>> > > > directly, > >>> > > > > >>> it might be "I hear your comment, and there are good > reasons > >>> to > >>> > do > >>> > > > > >>> otherwise" or simply saying - "I know it could be done this > >>> way > >>> > > but I > >>> > > > > >>> think otherwise" etc. etc. But sometimes I miss that there > is > >>> a > >>> > > > > >>> comment that I have not reacted to, I skipped it > unconsciously > >>> > etc. > >>> > > > > >>> > >>> > > > > >>> I think having "some" kind of reaction to comments and > >>> deliberate > >>> > > "I > >>> > > > > >>> believe the conversation is resolved" is a very good thing > and > >>> > > having > >>> > > > > >>> the author making a deliberate effort to "mark the > >>> conversation > >>> > as > >>> > > > > >>> resolved" is a sign it's been read, though about and > >>> consciously > >>> > > > > >>> reacted to. > >>> > > > > >>> > >>> > > > > >>> I've learned recently that you can add protection rule that > >>> will > >>> > > > > >>> require all conversations on PR to be resolved before > merging > >>> > it, I > >>> > > > > >>> even went to a great length to create (and get merged) a > PR to > >>> > ASF > >>> > > > > >>> infra to enable it via .asf.yml feature > >>> > > > > >>> (https://github.com/apache/infrastructure-p6/pull/1740) - > so > >>> we > >>> > > can > >>> > > > > >>> enable it now by a simple PR to our .asf.yaml enabling it. > >>> > > > > >>> > >>> > > > > >>> I'd love to try it - but of course it will have to change > a > >>> bit > >>> > > the > >>> > > > > >>> workflow of everyone, where the author (or reviewer, or > >>> > maintainer) > >>> > > > > >>> will have to mark all conversations as resolved > deliberately > >>> > before > >>> > > > > >>> merging PR. > >>> > > > > >>> > >>> > > > > >>> I'd love to enable it - at least to try and see how it can > >>> work - > >>> > > but > >>> > > > > >>> I understand it might add a bit of burden for everyone, > >>> however, > >>> > I > >>> > > > > >>> think it might be worth it. > >>> > > > > >>> > >>> > > > > >>> WDYT? > >>> > > > > >>> > >>> > > > > >>> J. > >>> > > > > >>> > >>> > > > > >>> > >>> > > > --------------------------------------------------------------------- > >>> > > > > >>> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>> > > > > >>> For additional commands, e-mail: > dev-h...@airflow.apache.org > >>> > > > > >>> > >>> > > > > >>> > >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > --------------------------------------------------------------------- > >>> > > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>> > > > > > For additional commands, e-mail: dev-h...@airflow.apache.org > >>> > > > > > > >>> > > > > > >>> > > > > > >>> --------------------------------------------------------------------- > >>> > > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>> > > > > For additional commands, e-mail: dev-h...@airflow.apache.org > >>> > > > > > >>> > > > > > >>> > > > > >>> > > > > >>> --------------------------------------------------------------------- > >>> > > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > >>> > > > For additional commands, e-mail: dev-h...@airflow.apache.org > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > >> >
