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

[WIP] Added documentation for handing pull requests #20

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DrakBoul
Copy link

In regards to #18
In this branch I added a markdown file that documents how to handle pull requests. It covers the following topics:

  • role of creator of pull request, such as how to write a good pull request
  • role of reviewer, requirements for completing a code review
  • defintions of when approved, request for changes, and comments and under what circumstances they should be used.

These guidelines and rules are based off of a couple of github articles:

  1. Reviewing proposed changes in a pull request
  2. How to write the perfect pull request

As the rules of reviewing pull requests affects all reviewers I am open to all feedback. Please let me know what you think of the definitions for approving, requesting changes, and commenting. Also let me know if the job of actually merging the branch should be the job of either the reviewer or the creator of the pull request, In this document I just said it could be either but it would be nice to have a concrete definition of roles.

…that it also covers guidelines for writing pull requests
Copy link

@rrasmuss4200 rrasmuss4200 left a comment

Choose a reason for hiding this comment

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

Also, maybe mention how one can make a PR a draft, then later on convert it into a full pull request.

Pull_Requests.md Outdated
\
The following are some general rules of how a reviewer should review a pull request:\
- If the PR is labelled as "WIP" then it should not be approved.
- Thoroughly go all changes made and if neccesary leave feedback.

Choose a reason for hiding this comment

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

Typo - 'go OVER all changes'

Copy link
Author

Choose a reason for hiding this comment

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

@rrasmuss4200 Oops, I didn't get an email notification for any of your comments so I did not see them until now. I'll give it another read over and fix the typos. Also I'll add a section for drafting PR's. Thanks for the feedback!

Copy link
Author

Choose a reason for hiding this comment

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

@rrasmuss4200 I fixed the typo and added a code styling section for rust (Using clippy and rust analyzer).

On a different note, in the contributing document there is still a section about using copyright information which I think may be out of date. I remember Devin mentioning that we no longer need to put the copyright info in each code file and now we just have a single license doc in the repo. Let me know if I should remove that section in this PR.

@DrakBoul DrakBoul requested a review from rrasmuss4200 January 8, 2025 16:21
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.

3 participants