Skip to content

GH-33619: [Documentation] Update PR template #33620

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.

Sign up for GitHub

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

Merged
jorisvandenbossche merged 3 commits into from
Jan 16, 2023
Merged

GH-33619: [Documentation] Update PR template #33620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53de09d
update pr template
assignUser Jan 11, 2023
9e03844
remove comments from commit message
assignUser Jan 16, 2023
7ff903d
fix regex
assignUser Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# Which issue does this PR close?

<!--
Thanks for opening a pull request!
Expand All @@ -25,22 +24,21 @@ In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

-->
Closes #

# Rationale for this change
### Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->

# What changes are included in this PR?
### What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->

# Are these changes tested?
### Are these changes tested?

<!--
We typically require tests for all PRs in order to:
Expand All @@ -50,7 +48,7 @@ We typically require tests for all PRs in order to:
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->

# Are there any user-facing changes?
### Are there any user-facing changes?
Copy link
Member

@wjones127 wjones127 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another piece of feedback:

  1. The breaking-change label didn't exist. I decided to add it as Breaking Change just now, since there isn't a good reason to kebab-case AFAIK.
  2. I don't think non-committers can apply labels, right? So perhaps the instructions simply need to have a checkbox [ ] This PR contains breaking changes or something like that?

Copy link
Member

@wjones127 wjones127 Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'll handle this in #33660


<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
Expand Down
2 changes: 2 additions & 0 deletions dev/merge_arrow_pr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,8 @@ def extract_co_authors(commit):
commit_title = f'{self.title} (#{self.number})'
commit_message_chunks = []
if self.body is not None:
# Remove comments (i.e. <-- comment -->) from the pull request description.
body = re.sub(r"<!--(.|\s)*-->", "", self.body)
# avoid github user name references by inserting a space after @
body = re.sub(r"@(\w+)", "@ \\1", self.body)
commit_message_chunks.append(body)
Expand Down