2010YOUY01 commented on code in PR #24051: URL: https://github.com/apache/datafusion/pull/24051#discussion_r3700935339
########## docs/source/contributor-guide/pr_review.md: ########## @@ -0,0 +1,223 @@ +<!--- + 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 alternate 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 a ticket), 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 run +`git log` / `git blame` and ask "why is the code like this?". + +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). + +[pr template]: https://github.com/apache/datafusion/blob/main/.github/pull_request_template.md + +## Review the Code Comments + +Well written code comments are what makes the codebase understandable to the +next contributor. + +Check that: + +1. The code has adequate comments, and that the comments focus on the + **rationale** for any non-obvious change (the "why"), not a restatement of + what the code does (the "what"), which is typically clear from reading the + code itself. +2. Comments do not narrate irrelevant internal implementation details or the + history of how the change was developed (this is common in LLM-assisted + code, e.g. "// changed to use a HashMap" or "// this handles the case + mentioned above"). Such comments become irrelevant as soon as the PR merges. +3. When comments refer to other structs, functions, or modules, they should use + [rustdoc intra-doc links] (e.g. `` [`SessionContext`] ``) rather than plain + text names, so that `cargo doc` link checking ensures the references stay + valid as the code evolves. +4. New public APIs have doc comments, including examples where appropriate + (doc examples are also tested by CI, so they double as test coverage). + Review Comment: ```suggestion 5. When documenting modules, functions, or fields, start with simple examples and intuitive explanations, and optionally add formal, math-like definitions when necessary. This makes the implementation easier to reason about. 6. When something is confusing on first read, treat that as a good opportunity to improve the comments. ``` Here are some extra items I find useful. ########## docs/source/contributor-guide/pr_review.md: ########## @@ -0,0 +1,223 @@ +<!--- + 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 alternate 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 a ticket), 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 run +`git log` / `git blame` and ask "why is the code like this?". + +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). + +[pr template]: https://github.com/apache/datafusion/blob/main/.github/pull_request_template.md + +## Review the Code Comments + +Well written code comments are what makes the codebase understandable to the +next contributor. + +Check that: + +1. The code has adequate comments, and that the comments focus on the + **rationale** for any non-obvious change (the "why"), not a restatement of + what the code does (the "what"), which is typically clear from reading the + code itself. +2. Comments do not narrate irrelevant internal implementation details or the + history of how the change was developed (this is common in LLM-assisted + code, e.g. "// changed to use a HashMap" or "// this handles the case + mentioned above"). Such comments become irrelevant as soon as the PR merges. +3. When comments refer to other structs, functions, or modules, they should use + [rustdoc intra-doc links] (e.g. `` [`SessionContext`] ``) rather than plain + text names, so that `cargo doc` link checking ensures the references stay + valid as the code evolves. +4. New public APIs have doc comments, including examples where appropriate + (doc examples are also tested by CI, so they double as test coverage). + +[rustdoc intra-doc links]: https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html + +## Review the Test Coverage + +Check that the feature or fix is covered sufficiently with tests (see the +[Testing](testing.md) guide for more details): the PR should include tests for +any new functionality, and a bug fix should include a test that reproduces the +reported problem. + +Guidelines for evaluating tests: + +1. Prefer `sqllogictest` (`.slt`) tests or DataFrame API tests where + possible, as they exercise **user visible behavior** and are less coupled + to internal implementation details than unit tests. Review Comment: We could insert a list item here like: ``` 2. Try to cover edge cases and failure scenarios as well, rather than testing only the common successful path. ``` ########## .ai/skills/pr_review/SKILL.md: ########## @@ -0,0 +1,33 @@ +--- +name: pr_review +description: Review Apache DataFusion pull requests following the project's PR review guide. Use whenever asked to review a DataFusion PR or PR URL, and whenever creating a PR, to check the changes against the same criteria before submitting. Review Comment: > absolutely agree for As you said, > > > I also strongly believe effective documentation should be written for both humans and agents so I purposely didn't make a specific skill for this (instead I made a skill that points at the relevant parts of the docs) > > Since AI will continue to evolve to mimic humans, I wonder if skills are strictly necessary. A contributor guide alone could be enough. However, these days, it does help route the AI's exploration path (assuming routing to the .ai/ directory works). I agree `skill`s are mostly the same as regular user doc. I think this `skills.md` has two part: 1. Coding agent specific instruction, like commands to run 2. Idea/philosophy behind the reviewing process And part 2 do point to an existing contributor doc. We should do something similar for other `skills.md`: try to explain the core ideas only in the user doc, `skills.md` should only reference it. ########## docs/source/contributor-guide/pr_review.md: ########## @@ -0,0 +1,223 @@ +<!--- + 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 alternate 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 a ticket), 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 run +`git log` / `git blame` and ask "why is the code like this?". + +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). + +[pr template]: https://github.com/apache/datafusion/blob/main/.github/pull_request_template.md + +## Review the Code Comments + +Well written code comments are what makes the codebase understandable to the +next contributor. + +Check that: Review Comment: ```suggestion The goal of code comments is not only for the code to be understandable after a reader spends time studying the implementation. It should make the implementation easy to understand for readers with the expected background. Some practical guidelines for writing comments: ``` Emphasizing the philosophy here might help -- 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]
