+1 on adding a section under CONTRIBUTING.md, everyone looks at it before starting contributing to the project.
In the template reminder, we can point devs to CONTRIBUTING.md for more info. Thanks, Christos On Thu, Jul 27, 2023 at 2:48 AM Ethan Rose <er...@cloudera.com.invalid> wrote: > 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 > > > > > > > > > >
