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

GitHub policies #740

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

GitHub policies #740

wants to merge 10 commits into from

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented Sep 15, 2021

Policies and best practices for Github

GitHubPolicy.md Outdated Show resolved Hide resolved
GitHubPolicy.md Outdated
> If you see any mistakes, just add a comment in github instead of going in and trying to fix it yourself. This keeps the edit history clean and unambigous as to where the contribution came from.

* Be timely
> When reviewing, make sure to review the entire PR. Commenting on part of the code, while not looking at other parts of the code may lead to confusion down line. Try to address any replies from the author within 48 hours. This keeps it fresh for yourself and the contributor, making it easier to understand the comments.
Copy link
Member

@achou11 achou11 Sep 15, 2021

Choose a reason for hiding this comment

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

I like the hard number here but wondering if we should be that rigid about it vs saying something like "in a timely manner" 🤷‍♂️ guess it depends on how specific we want these guidelines to be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Good point, I am trying to imply that they should TRY. I can change the wording to make it seem more like a suggestion then a hard fast rule

GitHubPolicy.md Outdated
> Make sure that you have fully tested your code on a device or emulator. It is up to you as the author, to make sure everything is functioning the way that you intended.

* Be Timely.
> After someone has reviewed you PR and left comments, try to address those comments within 48 hours. This keeps it fresh for yourself and the reviewer, making it easier to understand the comments.
Copy link
Member

Choose a reason for hiding this comment

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

similar comment about using hard numbers for this

GitHubPolicy.md Outdated
Comment on lines 117 to 120
> Add Screen shots of the new UI
>
> # Wireframes
> Include link to wireframes here
Copy link
Member

@achou11 achou11 Sep 15, 2021

Choose a reason for hiding this comment

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

i think we can generalize this to something like Include any relevant screenshots or links maybe with a qualifier along the lines of e.g. screenshots of changes in UI. I'm assuming that this is a resource that should apply to a broader scope of contributors, whether they're outside the team or contributing to the backend (could be mistaken)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I want to separate screenshots and wireframes. I think people should show screenshots regradless AND if they have wireframe, then to refer to them here. So I will make it more obvious that they should include it if they have it!

> Include any tasks that still need to be completed that cannot be addressed in this PR
GIVE EXAMPLES HERE

## How To
Copy link
Member

@achou11 achou11 Sep 15, 2021

Choose a reason for hiding this comment

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

this might be an iffy section because it'll largely vary based on the git interface they're using (e.g. GitHub's desktop app, git cli, gh cli client, etc). generally would point to official documentation about each thing if available (i'm sure GitHub has some pages dedicated to some of these)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so I think the majority of people will be using browser, and if not, then I think they can copy and paste it off this document. I think there is a lot of value to having a template.

Copy link
Member

Choose a reason for hiding this comment

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

so would you just list CLI commands in this section? My point is that you can have many variations of each step depending on the interface that's being used, which will make this document pretty verbose if you try to document all of them.

Co-authored-by: Andrew Chou <[email protected]>
* Make sure test passes CI.
>Our CI tests take a few minutes to run after a PR has been created in Github. Check to make sure that these tests have passed. If they did not pass, see troubleshooting (TODO).

### **Reviwing PR**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### **Reviwing PR**
### **Reviewing PR**

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