Hi Eric I agree. I propose:
1. To update the contributor good practices on the website to clearly state the keywords nit and minor. 2. For request change, we can use it. For the record, we can bypass request change flag if a given number of reviewers approve the PR anyway. Thoughts ? Regards JB Le mar. 25 mars 2025 à 19:54, Eric Maynard <eric.w.mayn...@gmail.com> a écrit : > 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. > > > > > > > > > > >
