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