Create pull request template

[sameh-farouk]

Related Issue:

• add issue and pr templates to the repo #849

[brandonpille]

This checklist also seems overkill to me. Although the bullet point about the documentation is relevant. It would make more sense to put it into the issue also. Same for tests. The last bullet point should also not be there. We should not be allowed to merge a PR if the tests are failing. Making the branch protected should handle that.

[sameh-farouk]

Removed the pass tests point.
I think other points are relevant. no?

[sameh-farouk]

People tend to forget. I’ve seen pull requests that are missing required migrations, pull requests that are missing documentation, and pull requests that don’t follow the commit conventions. This is why the checklist makes sense to me.

[brandonpille]

Yes it does make sense but I would put it into the issue. These bullet points are dependent on the issue no? Not every issue requires tests (documentation changes for example). Not every issue needs documentation (bug fixes to tools for example).

So maybe you can create a template for issues? People who create issues will have to remove the bullet points that are not needed and will have to add a description for the issue. Then it's up to the reviewer to double check the requirements vs what has been delivered. What do you think?

[sameh-farouk]

I’m not sure that forcing these points into the issue is more reasonable.
Anyone can open issues, even non-technical users. Issues are usually about something that doesn’t work or about suggestion, and the changes required to sort this out could be unclear from the issue alone or unknown to the user. It could be that the user is not able to understand how to make it work, so the developer adds documentation. It could be an actual bug that requires a fix or something that is not even supported and if accepted requires a new feature. So, an issue usually is about a problem not about the changes requires to fix this problem and a PR should include the details on how we fixed this problem.

A good pull request is one that you understand the point of, clarifies the intentions of the code changes, summarizes the code well and identifies some of the thought process involved in writing the code.

Also, a great pull request answers some questions before they are asked.

[brandonpille]

Everyone can indeed open issues. Maybe you're right and maybe it should go in the PR. Though I would remove some bullet points:

• My code follows the code style of this project.
• I have read the CONTRIBUTING document.
• All new and existing tests passed.

For these reasons:

• Your code should always follow the code style of the project
• Everyone should have read that document before working on tfchain and once you have read it you will have to check that box for every PR which is a bit of a hassle
• The tests should always pass before merging and we enforce that by not being able to merge the PR

Also these two bullet points go hand in hand, I would only put one that says both:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.

Maybe rewrite to:

• My changes requires a change to the documentation and I have updated it

My point here is that it should not be hassle to create PRs. There should be an issue with clear requirements and then the PR should point to it and contain some implementation details/description. The goal of a PR is to allow other people to review your work and to do so you will need the issue and maybe some more details. A good reviewer will know that he has to check for tests, documentation, code style and that the tests are passing.

[sameh-farouk]

Can you check now?

[renauter]

I think all points you put are relevant, great work!
Meanwhile this looks to me more like a PR recommendation instead of a template since there are a lot of informations on it that could (at least partially) be in the issue.
When I read a PR description I expect to go quickly straight to the point if I am a reviewer
When I write a PR description I don t want to spend too much time on evaluating all bullets on it (I just need to be aware of)
And it also will add extra layer of evaluation by evaluating the PR description (such bullet is not checked and it should be)
But I agree there are some critical points such as migration and documentation update that should be specified
So let s try like this and we will adapt as needed or we find an lighter alternative (PR => what issue it solves and whatever related to code) ?

[sameh-farouk]

I tried to incorporate all your feedback and i think we are getting there.
I can also remove the last two points (commits style and release guide) to simplify the check-boxes to only three points and removes the less critical details. Is this better? @brandonpille @renauter

[brandonpille]

Looks good! Thank you for the changes.