-
Couldn't load subscription status.
- Fork 3.9k
GH-47899: [Dev] Add checklist to PR template #47916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
|
|
||
| ### Checklist | ||
|
|
||
| - [ ] The PR's title follows existing conventions of the repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a link to the Contributing Overview in the beginning of the template, but some extra info about what the title should look like might be good? Maybe adding a link again (https://arrow.apache.org/docs/developers/overview.html#pull-request-and-review) or an example.
Because I find titles needing an update quite frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link would be good.
| **This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.) | ||
|
|
||
| **This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you keep this in the "Are there any user-facing changes?" section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to other comments others have made, left one change.
I'm +1 to this since you and others do the majority of the PR review and triage.
| ### Checklist | ||
|
|
||
| - [ ] The PR's title follows existing conventions of the repository | ||
| - [ ] My code follows the general style and conventions of the codebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - [ ] My code follows the general style and conventions of the codebase | |
| - [ ] My code follows the general style and conventions of the codebase ([C++](https://arrow.apache.org/docs/developers/cpp/development.html), [Python](https://arrow.apache.org/docs/developers/python/development.html)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a good single reference for code style, but maybe linking directly to the C++ and Python development pages (which include style guidance) would be good? Generally, it'd be nice if as much of the contents of the PR template had links.
|
|
||
| ### Checklist | ||
|
|
||
| - [ ] The PR's title follows existing conventions of the repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the two existing claims should be above the checklists as kou pointed out, lgtm.
Rationale for this change
This is to try to steer contributors into submitting more polished PRs.