You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/06/05 20:05:41 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6546: Minor: add more PR guidance in contributing guide
alamb opened a new pull request, #6546:
URL: https://github.com/apache/arrow-datafusion/pull/6546
# Which issue does this PR close?
N/A
# Rationale for this change
I have had PR size discussions with various people over time and I wanted to write down suggestions so that
1. We could agree as a community on the guidance (so it was consistent)
2. It can be clearly documented and we don't have to repeat it on a case by case basis
Most recently this came up with @izveigor on https://github.com/apache/arrow-datafusion/pull/6384 but I have had similar exchanges with @ozankabak and likely others
# What changes are included in this PR?
Add a suggestion in the contributor section of the docs
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
N/A
# Are there any user-facing changes?
Docs
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on pull request #6546: Docs: add more PR guidance in contributing guide (smaller PRs)
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #6546:
URL: https://github.com/apache/arrow-datafusion/pull/6546#issuecomment-1577811138
LGTM
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] r4ntix commented on a diff in pull request #6546: Docs: add more PR guidance in contributing guide (smaller PRs)
Posted by "r4ntix (via GitHub)" <gi...@apache.org>.
r4ntix commented on code in PR #6546:
URL: https://github.com/apache/arrow-datafusion/pull/6546#discussion_r1218795543
##########
docs/source/contributor-guide/index.md:
##########
@@ -41,11 +41,29 @@ DataFusion is a very active fast-moving project and we try to review and merge P
Review bandwidth is currently our most limited resource, and we highly encourage reviews by the broader community. If you are waiting for your PR to be reviewed, consider helping review other PRs that are waiting. Such review both helps the reviewer to learn the codebase and become more expert, as well as helps identify issues in the PR (such as lack of test coverage), that can be addressed and make future reviews faster and more efficient.
-Things to help look for in a PR:
+## Creating Pull Requests
+
+We recommend splitting your contributions into smaller PRs rather than large PRs (500+ lines) because:
+
+1. The PR is more likely to be reviewed quickly -- our reviewers struggle to find the contiguous time needed to review large PRs.
+2. The PR discussions tend to be more focused and less likely to get lost among several different threads.
+3. It is often easier to accept and act on feedback when it comes early on in a small change, before a particular approach has been polished too much.
+
+If you are concerned that a larger design will be lost in a string of small PRs, creating a large draft PR that shows how they all work together can help.
+
+# Reviewing Pull Requests
+
+When reviewing PRs, please remember our primary goal is to improve DataFusion and its community together. PR feedback should be constructive with the aim to help improve the code as well as the understanding of the contributor.
+
+Please ensure any issues you raise contains a rationale and suggested alternative -- it is frustrating to be told "don't do it this way" without any clear reason or alternate provided.
Review Comment:
❤️
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jackwener merged pull request #6546: Docs: add more PR guidance in contributing guide (smaller PRs)
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener merged PR #6546:
URL: https://github.com/apache/arrow-datafusion/pull/6546
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org