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

Pull Request Template #885

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Pull Request Template #885

merged 1 commit into from
Mar 14, 2024

Conversation

capnramses
Copy link
Collaborator

@capnramses capnramses commented Feb 28, 2024

Reason for this PR

Detailed in #856 , linked.

A standardised PR template can help:

  • Manage expectations of contributors.
  • Onboarding team members, and external users learn the codebase and understand new features.
  • Reviewers understand the "why" or reasoning and approach behind the work.
  • Add to insight and tracking of changes over time. This helps with bug hunting, regression analysis, and refactoring "is this code still needed?".
  • Highlight risks of various categories that might otherwise slip through the review process.
  • Add a consistent element to quality control by using a checklist.

Related reading:
https://luminousmen.com/post/github-pull-request-templates
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/best-practices-for-pull-requests

Changes Made in this PR

  • Added pull_request.md file which GitHub looks for in any folder and will present on opening a PR in the interface. I put in in .github/ just to throw it in the same folder with Issue templates etc.
  • To demonstrate how the format looks I pasted the template into this PR description. If you view the src you'll see the guiding comments also.
  • I based the contents on best practices suggestions, looked at other repositories with good PR templates, and all our related repositories, and basically did a max() of the good bits in all of them.

Testing Summary

Not tested. No code. Might need to run pre-commit if there's a stray whitespace in the markdown.

Risk Highlight

  • This PR includes code from another work (please detail).
  • This PR contains API-breaking changes.
  • This PR depends on work in another PR (please provide links/details).
  • This PR introduces new dependencies (please detail).
  • There are coverage gaps not covered by tests.
  • Documentation updates required in subsequent PR.

Checklist

  • Code comments added to any hard-to-understand areas, if applicable.
  • Changes generate no new warnings.
  • Updated any relevant tests, if applicable.
  • No conflicts with destination dev branch.
  • I reviewed my own code changes.
  • Initial CI/CD passing.
  • 1+ reviews given, and any review issues addressed and approved.
  • Post-review full CI/CD passing.

Future Work

  • When we have a public Contributor Guide markdown explaining the PR process expecations, we should update this template and link that in here.

@capnramses capnramses self-assigned this Feb 28, 2024
@capnramses capnramses linked an issue Feb 28, 2024 that may be closed by this pull request
11 tasks
@nickfraser
Copy link
Collaborator

This is excellent - thank you! 💪

Copy link
Collaborator

@costigt-dev costigt-dev left a comment

Choose a reason for hiding this comment

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

Looks good - should capture a nice bit of detail

@capnramses capnramses merged commit 1a396fe into Xilinx:dev Mar 14, 2024
344 of 347 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GitHub PR Template
3 participants