In this case I think rather than a mistake, there could simply be some ambiguity around what comments are considered blocking. For example, I'm not sure if a comment prefixed "minor" should be considered blocking -- I would say probably not, but someone else could interpret it differently.
To that end, I think putting these things in the guidelines could be wise even if there's not a strict enforcement of the guidelines as such. I agree that it's hard to "codify a spirit", but it could be useful to have explicit expectations and guidelines written down somewhere. It could be as simple as asking reviewers to "Request Changes" if they want to block a PR, or advising authors to wait a couple of days before merging a PR they view as complex. --EM On Mon, Mar 24, 2025 at 11:13 AM Yufei Gu <flyrain...@gmail.com> wrote: > I'm particularly concerned about what happened in PR #1230( > https://github.com/apache/polaris/pull/1230). The PR was merged despite > multiple unresolved comments from a committer. > > To maintain a healthy review process, we’d like to encourage PR authors to > address open comments and work toward consensus. If necessary, relying on > lazy consensus is fine. > > Let’s all try to be mindful of this going forward to ensure smooth > collaboration. > > Yufei > > > On Mon, Mar 24, 2025 at 1:55 AM Jean-Baptiste Onofré <j...@nanthrax.net> > wrote: > > > Hi Eric, > > > > Thanks for pointing this out. > > > > I think that's unfortunate, especially for #1230 (I don't see issues > > on #1226 and #1220 as they have been approved by 3 different > > committers). > > > > I strongly believe that, as a community, we do a great work all > > together, and we highly consider comments from each other. > > If again, I think it's unfortunate for #1230, it's certainly a > > "mistake" from the author/merger and we should still give a change to > > our soft rules (before being stricter). > > > > So, I propose to continue with our soft rules and good intentions, and > > if it doesn't work, then we can discuss about a stricter approach. > > > > Thoughts ? > > > > Regards > > JB > > > > On Sun, Mar 23, 2025 at 8:05 AM Eric Maynard <eric.w.mayn...@gmail.com> > > wrote: > > > > > > Revisiting this thread, I wonder if the "soft rules" are working. I > have > > > noticed quite a few PRs merged recently with outstanding comments. The > > most > > > recent of these that I personally reviewed are #1220 > > > <https://github.com/apache/polaris/pull/1220>, #1226 > > > <https://github.com/apache/polaris/pull/1226>, and #1230 > > > <https://github.com/apache/polaris/pull/1230> but there are doubtless > > other > > > examples. > > > > > > If, indeed, the guidelines are not working perhaps stricter enforcement > > > more consistent with JB's initial proposal would be effective. > > > > > > On Fri, Jan 24, 2025 at 8:21 AM Dmitri Bourlatchkov > > > <dmitri.bourlatch...@dremio.com.invalid> wrote: > > > > > > > On Thu, Jan 23, 2025 at 5:15 AM Jean-Baptiste Onofré < > j...@nanthrax.net> > > > > wrote: > > > > > > > > > Let's rename from "guidelines" to "good practices & advices" :) > > > > > > > > > > > > > +1 to that. I'm not sure it worth trying to "codify a spirit" (from > > > > previous emails), but > > > > I think having good advice is helpful to instill the spirit of > > goodwill and > > > > collaboration > > > > into the community. > > > > > > > > In this regard: > > > > > - I propose to create a PR to update the contributing guide with > this > > > > > good practices discussed in this thread > > > > > > > > > > > > > +1 > > > > > > > > - I propose to close https://github.com/apache/polaris/pull/840 in > > > > > favor of "soft rule" (we quickly discuss about owner review with > > > > > > > > > > > > > +1 > > > > > > > > CODEOWNERS in this PR, but let's keep aside for now) > > > > > - I propose to update https://github.com/apache/polaris/pull/839. > We > > > > > have a consensus about the label, but no consensus about the > schedule > > > > > for now. In order to move forward, I propose to update the PR with > > > > > just the label for now (as we have a consensus) and see if it's > > > > > enough. If it's still a concern, I will create a new PR to update > the > > > > > schedule > > > > > > > > > > > > +1 > > > > > > > > Cheers, > > > > Dmitri. > > > > > > >
