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.

Sign up for GitHub

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

Jump to bottom

[No QA] Add proposal template for contributors #14817

Merged
merged 8 commits into from
Feb 7, 2023
Merged

[No QA] Add proposal template for contributors #14817

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37a130c
Add proposal template for contributors
marcaaron Feb 3, 2023
8b33dbe
instructions
marcaaron Feb 3, 2023
ceb1c0b
Update CONTRIBUTING.md
marcaaron Feb 3, 2023
ca70861
Update CONTRIBUTING.md
marcaaron Feb 3, 2023
03e76a1
Update CONTRIBUTING.md
marcaaron Feb 3, 2023
7d71198
Update CONTRIBUTING.md
marcaaron Feb 3, 2023
36ed1cf
Add @tgolen suggestions. Encourage contributors to update their exist…
marcaaron Feb 6, 2023
edfcbfb
Tell people the reason for posting a comment after updating
marcaaron Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions contributingGuides/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ Additionally if you want to discuss an idea with the open source community witho
3. If you cannot reproduce the problem, pause on this step and add a comment to the issue explaining where you are stuck or that you don't think the issue can be reproduced.

#### Propose a solution for the job
4. After you reproduce the issue, make a proposal for your solution and post it as a comment in the corresponding GitHub issue (linked in the Upwork job). Your solution proposal should include a brief written technical explanation of the changes you will make. Include "Proposal" as the first word in your comment.
4. After you reproduce the issue, complete the [proposal template here](./PROPOSAL_TEMPLATE.md) and post it as a comment in the corresponding GitHub issue (linked in the Upwork job).
- Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. Any new proposal should be substantively different from existing proposals.
5. Pause at this step until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet).
6. If your solution proposal is accepted by the Expensify engineer assigned to the issue, Expensify will hire you on Upwork and assign the GitHub issue to you.
7. Once hired, post a comment in the Github issue stating when you expect to have your PR ready for review
5. Refrain from leaving additional comments until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet).
6. If your proposal is accepted by the Expensify engineer assigned to the issue, Expensify will hire you on Upwork and assign the GitHub issue to you.
7. Once hired, post a comment in the Github issue stating when you expect to have your PR ready for review.
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting-your-thoughts: Should we say something about only posting one proposal at a time? Oftentimes I see someone will come along and post up to as many as four different proposals. On high-traffic issues, it's a lot to manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good call out. I think ideally a contributor should only have one "proposal waiting for a review" at a time. Sometimes it happens that you come up with alternate solutions. But I'd argue those belong back the proposal you have open and you should update the current proposal if it hasn't been reviewed yet.

Which also makes me wonder whether we want people to "update their original proposal" or post a new one (since people don't all do this the same way).

Thinking more on all of this and I think my ideal would probably be something like:

  • Always wait for a response to your current proposal before leaving additional proposals.
  • It's OK to update a proposal if it hasn't been reviewed yet.
  • If you're making a new proposal or updating a proposal then edit your original proposal and leave a comment like:
## Proposal

[Updated](link to proposal)

Copy link
Contributor Author

@marcaaron marcaaron Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Always wait for a response to your current proposal before leaving additional proposals.

maybe that is unclear though. because we are saying it's ok to leave additional proposals. I think it would be better to just have one proposal that the contributor is tweaking and evolving until it's perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe something like...

  • Do not leave more than one proposal.
  • Do not make extensive changes to your current proposal until after it has been reviewed.
  • If you want to make an entirely new proposal or update an existing proposal please go back and edit your original proposal and leave a comment like this:
## Proposal
[Updated](link to proposal)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of always keeping 1 post per proposal and only having one proposal reviewed at a time. I don't quite get the link to proposal when updating an existing one. If it's all in the same post, then there isn't anything to link to. Plus, GH keeps the edit history of the post anyway.

The thing I dislike is when there is one large proposal that keeps getting tweaked, and so the tweaked proposal exists in several different forms across comments and it's difficult to see what changed between them.

That's why I like just always keeping it in a single post and making updates to the existing post rather than posting a new comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's all in the same post, then there isn't anything to link to

Ah, so in my head it was a link to the comment which "jumps" you to the proposal in the same issue.

I think the idea with posting an update to a proposal is just that it encourages the best practice of letting everyone know your proposal is "ready for review" again. And then we'd have a general loop of:

  • Propose
  • Incorporate feedback
  • Update proposal
  • Request re-review
  • Etc.

This probably happens organically already (perhaps inconsistently) so maybe the attempt to create a process for this seems pointless.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I got you. Just like how we post "updated" on a PR to let people know it can be reviewed again. I like that


#### Begin coding your solution in a pull request
7. When you are ready to start, fork the repository and create a new branch.
Expand Down
32 changes: 32 additions & 0 deletions contributingGuides/PROPOSAL_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
## Proposal

### Please re-state the problem that we are trying to solve in this issue.

### What is the root cause of that problem?

### What changes do you think we should make in order to solve the problem?
marcaaron marked this conversation as resolved.
Show resolved Hide resolved

### What alternative solutions did you explore? (Optional)

**Reminder:** Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

<!---
ATTN: Contributor+

You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we at a minimum understand root cause even if the solution doesn't directly address it. When we avoid this step we can end up solving the wrong problems entirely or just duct taping over them.
marcaaron marked this conversation as resolved.
Show resolved Hide resolved

Instructions for how to review a proposal:

1. Address each contributor proposal one at a time and address each part of the question one at a time e.g. if a solution looks acceptable, but the stated problem is not clear then you should provide feedback and make suggestions to improve each prompt before moving on to the next. Avoid responding to all sections of a proposal at once. Move from one question to the next each time asking the contributor to "Please update your original proposal and tag me again when it's ready for review".

2. Limit excessive conversation and moderate issues to keep them on track. If someone is doing any of the following things please kindly and humbly course-correct them:

- Posting PRs.
- Posting large multi-line diffs (this is basically a PR).
- Skipping any of the required questions.
- Not using the proposal template at all.
- Suggesting that an existing issue is related to the current issue before a problem or root cause has been established.
- Excessively wordy explanations.

3. Choose the first proposal that has a reasonable answer to all the required questions.
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting-your-thoughts: Should we also say something that it's OK to hide comments if they are off-topic or no longer need to be considered? I think that can be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like, maybe if someone doesn't use the proposal template, or they haven't completed all the questions, those are good candidates to be hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this, but I'm not sure if C+ has moderation tools available.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, they do not have that. Do you have any idea if it's easily possible to give them moderation ability? I don't know much about how fine-grained the settings are. CC @johnmlee101 or @coleaeason if you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I just looked this up and there actually is a concept of a "moderator" in GitHub:

2023-02-06_11-09-22

https://docs.github.com/en/organizations/managing-peoples-access-to-your-organization-with-roles/managing-moderators-in-your-organization#about-organization-moderators

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds perfect!