That sounds good to me. We could add a brief reminder there and more details in the contributing guide if we want.
On Wed, Jul 26, 2023 at 4:31 PM Wei-Chiu Chuang <weic...@apache.org> wrote: > Actually I'd prefer adding a reminder in the PR template here > > https://github.com/apache/ozone/blob/master/.github/pull_request_template.md > Seems like a good way to soft-enforce the rule. > > On Wed, Jul 26, 2023 at 3:53 PM Ethan Rose <er...@cloudera.com.invalid> > wrote: > > > I've definitely been on both sides: posting a large PR myself, and as a > > reviewer asking for PRs to be separated into smaller pieces. If we want > > something persisted beyond an email thread I think we could update > > CONTRIBUTING.md with a new section about how to post larger changes. Tips > > for breaking down PRs that we could document: > > - Separating refactoring PRs from the new change that depends on the > > refactoring > > - Splitting one refactoring of a general area of code into individual > > refactoring PRs to iterate from the current state to the desired state. > > - Use parent jiras with subtasks as part of planning, before coding. > > - Provide design docs or at least detailed PR descriptions to make > complex > > reviews more manageable. > > - A large PR with a description covering multiple disjoint issues may be > > better split to independent fixes. > > > > I mention refactoring a lot because those sorts of PRs are frequent > > culprits of these large diffs. One example I had recently was refactoring > > in https://github.com/apache/ozone/pull/4838 followed by the change > > dependent on the refactoring in > https://github.com/apache/ozone/pull/4867. > > The latter was still a large PRs but splitting helped speed up review and > > keep the diff under 1k lines. Made my dev work easier too. > > > > I don't think a large PR should warrant a CI failure either, this seems > > like the type of thing that would be up to reviewers to enforce since it > > impacts them the most. If you're set to review a large change, ask if > there > > is any way it can be split to a dependency chain of two or more > consecutive > > PRs. Having something written in COMTRIBUTING.md will provide a source > that > > reviewers can point devs towards as a reference in these situations. > > > > Ethan > > > > On Wed, Jul 26, 2023 at 3:02 PM Wei-Chiu Chuang <weic...@apache.org> > > wrote: > > > > > Hi Ozoners, > > > > > > In one of the coffee break chats with a colleague of mine, we realized > > many > > > of the PRs in the Ozone project are quite lengthy. > > > > > > I'm guilty of this myself too. Keeping PR short and sweet is good > > hygiene. > > > It allows reviewers to spot potential problems in the code easier, and > > your > > > PR is more likely to be reviewed and iterated quickly. > > > > > > How would you like to see the PR quality improved? I'd like to urge > > > everyone to break down PRs but I don't necessarily want a GitHub Action > > > that enforces length limit. :) > > > > > > Weichiu > > > > > >
