Skip to content
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

GH-2939: Update PULL_REQUEST_TEMPLATE #2940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wgtmac
Copy link
Member

@wgtmac wgtmac commented Jul 3, 2024

Rationale for this change

The original PR template is a little bit out of date, especially after moving from JIRA to Github issue.

What changes are included in this PR?

Update PULL_REQUEST_TEMPLATE to remove unnecessary check boxes and follow what Apache Arrow does.

Are these changes tested?

This is not a code change.

Are there any user-facing changes?

No.

Closes #2939

@wgtmac
Copy link
Member Author

wgtmac commented Jul 3, 2024

@rok @gszadovszky @Fokko @julienledem Would you mind taking a look?

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @wgtmac

I left a few comments. My personal preference is that these PR templates should be as short and concise as possible. I think that our old template is already a bit on the long side. My feeling is that people don't read them when they become too long. That's why I'm a bit of a nit-pick on the wording, sorry for that. I also suggested removing the markdown from the comments, since they won't render anyway.

Let me know what you think!

### Issue
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're new to Parquet-Java, information on how to contribute can be found here: https://parquet.apache.org/docs/contribution-guidelines/contributing

- https://github.com/apache/parquet-java/issues/1234
- In case you are adding a dependency, check if the license complies with
the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
If this is not a minor PR, could you open an issue for this pull request on GitHub? https://github.com/apache/parquet-java/issues/new/choose
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If this is not a minor PR, could you open an issue for this pull request on GitHub? https://github.com/apache/parquet-java/issues/new/choose
when contributing large features, open an issue for this pull request on GitHub: https://github.com/apache/parquet-java/issues/new/choose


### Tests
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Parquet-Java project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Parquet-Java project.
Opening GitHub issues ahead of time ensure openness of the Apache Parquet-Java project, read more about this here: http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.


- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Then could you also rename the pull request title in the following format?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Then could you also rename the pull request title in the following format?
Please format pull request title in the following format:

-->
<!-- **This PR includes breaking changes to public APIs.** -->

<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

I might want to leave the critical fix out here. I dislike long PR templates, and I find that people don't really read them :'( Less is more I would say, or we should link to existing content (for example, link to the ASF Security page: https://www.apache.org/security/).

Copy link
Contributor

@gszadovszky gszadovszky left a comment

Choose a reason for hiding this comment

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

Thanks @wgtmac for doing this. I agree with @Fokko's comments, otherwise LGTM.

@wgtmac
Copy link
Member Author

wgtmac commented Jul 3, 2024

Thanks for the suggestion! I've simplified the wording and removed unimportant comments. PTAL. @Fokko @gszadovszky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update PULL_REQUEST_TEMPLATE
3 participants