2010YOUY01 commented on code in PR #24051: URL: https://github.com/apache/datafusion/pull/24051#discussion_r3725531884
########## docs/source/contributor-guide/pr_review.md: ########## @@ -0,0 +1,234 @@ +<!--- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Reviewing Pull Requests + +When reviewing PRs, our primary goal is to improve DataFusion and its community +together. PR feedback should be constructive and help improve the code as well +as the understanding of the contributor. + +Review bandwidth is currently our most limited resource, and reviews from the +broader community are both welcomed and encouraged. Reviewing PRs is a great way +to learn the codebase, and you do not need to be a committer to leave valuable +review feedback. In fact, one of the best ways to become a committer is to +thoughtfully review other PRs. + +Please ensure any comments you leave contain a rationale and suggested +alternative -- it is frustrating to be told "don't do it this way" without any +clear reason or alternative provided. + +The criteria in this guide are also a useful checklist when preparing your own +PR for review. + +## PR Review Mechanics + +Some helpful links: + +- [PRs Waiting for Review] on GitHub +- [Approved PRs Waiting for Merge] on GitHub + +[prs waiting for review]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+-review%3Aapproved+-is%3Adraft+ +[approved prs waiting for merge]: https://github.com/apache/datafusion/pulls?q=is%3Apr+is%3Aopen+review%3Aapproved+-is%3Adraft + +The overall PR lifecycle (CI triggering, approval, the 24-hour rule for +"major" PRs, and merging) is described in the +[Pull Request Overview](index.md#pull-request-overview) section of the +contributor guide. + +Practical tips: + +1. Check out the changes locally to explore them in your IDE or with an + agent, e.g. `gh pr checkout <PR number>` using the [GitHub CLI]. +2. There is normally no need to rerun tests locally that CI has already run. +3. Leave comments on specific lines of the diff where possible, so the + discussion has context. +4. If you review a PR but don't feel confident approving it, leaving comments + is still valuable: a partial review (e.g. "I reviewed the tests and they + look good") helps the next reviewer focus their time. +5. Anything that does not need to block the current PR can be noted as a + potential follow-up (ideally by filing an issue), keeping the PR focused + and quick to merge. + +[github cli]: https://cli.github.com/ + +## Review the PR Description + +The PR description is often what users and contributors will find when they have +a question about the intention behind a change, or when the code itself is not +clear. The PR description also becomes the extended commit message. + +Check that the description: + +1. Concisely describes the **problem being solved from the user's point of + view**. + +2. Follows the [PR template], and answers the template's questions. + +3. Accurately describes what the PR actually does. + +4. Explicitly calls out any user-facing or API changes (see + [Review the Code](#review-the-code) below). + Review Comment: > Keeping in mind that the PR description also becomes the commit message, which IMO is more useful as higher-level than lower-level, one can also use other means to provide more details on the implementation for reviewers: I agree commit message should be a concise 'what' summary, however I think making the PR description as the commit message is not a good default configuration, I'll try to find is there any better settings, like adding a 'summary' section in PR template, and only use its content as the commit message. > A bit more concretely, the PR template asks "What changes are included in this PR?". What do we expect people to answer here and at what level of detail? The template says "a summary of the individual changes in this PR". I always interpreted that as the key changes to the code that were made. My personal habbit is - try to explain most rationale in code comment - In PR description only provide tldr and pointers (e.g. 'please first read code comment at `file A` and `struct B`, then follow along to understand the entire PR') This way there will be no hidden info that only exist in PR writeup, and people reading related code from somewhere else won't miss anything; also PR description can further help reviewers to understand. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
