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

Update CONTRIBUTING.md #1715

Merged
merged 5 commits into from
Mar 19, 2021

Conversation

michaelhaxhiu
Copy link
Contributor

@michaelhaxhiu michaelhaxhiu commented Mar 11, 2021

Work in progress on updating the Contributor Guidelines

Details

Contributor.md copy updates that we shared in Slack here

Fixed Issues

https://github.com/Expensify/Expensify/issues/154933

Work in progress on updating the Contributor Guidelines
@michaelhaxhiu michaelhaxhiu requested a review from a team as a code owner March 11, 2021 00:34
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@botify botify requested review from pecanoro and removed request for a team March 11, 2021 00:35
@pecanoro
Copy link
Contributor

Hax, the little bot wants you to sign the CLA before I can merge by following what it says here #1715 (comment)

@michaelhaxhiu
Copy link
Contributor Author

michaelhaxhiu commented Mar 11, 2021

Rocio, thanks for defending me from the bots! We aren't ready to merge this juuuust yet, but I will take you up on a review when it's finalized (if you are offering it 😄 )

CONTRIBUTING.md Show resolved Hide resolved
- Please do NOT ping individual reviewers in the Slack channel to get their attention and keep the conversation in GitHub whenever possible.
- Patience is a virtue. Reviews can sometimes take place over the course of several days. If your PR has not been addressed after 3 days please leave a comment on the PR. If the PR goes without a response for another day please let us know via Slack.

### JavaScript Style
Copy link
Contributor

Choose a reason for hiding this comment

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

CONTRIBUTING.md Show resolved Hide resolved
Copy link
Contributor

@marcaaron marcaaron 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, but I think we have leaned this doc out a little too much

CONTRIBUTING.md Outdated
@@ -2,59 +2,64 @@
Welcome! Thanks for checking out Expensify.cash and for taking the time to contribute!

## Getting Started
This guide is specifically for external contributors. For a general overview of the repo, check out our README located [here](https://github.com/Expensify/Expensify.cash/blob/master/README.md). The part of the README to pay particular attention to is how to [run the app](https://github.com/Expensify/Expensify.cash#local-development) locally using our production API.
This guide is for external contributors. Check out our README guidelines here for a general overview of the code repository, how to run the app locally, testing, storage, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

This guide is for external contributors.

NAB, this is sort of implied since it's a CONTRIBUTING.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the phrasing to be more accurate. Hopefully it's better now?

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
- Please do NOT ping individual reviewers in the Slack channel to get their attention and keep the conversation in GitHub whenever possible.
- Patience is a virtue. Reviews can sometimes take place over the course of several days. If your PR has not been addressed after 3 days please leave a comment on the PR. If the PR goes without a response for another day please let us know via Slack.

### JavaScript Style
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in Slack, I'd like to see this stuff back in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, adding back

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@michaelhaxhiu michaelhaxhiu removed the request for review from pecanoro March 16, 2021 16:08
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple of requests


## Code of Conduct
This project and everyone participating in it is governed by the Expensify [Code of Conduct](https://github.com/Expensify/Expensify.cash/blob/master/CODE_OF_CONDUCT.md). By participating, you are expected to uphold this code. Please report unacceptable behavior to [contributors@expensify.com](mailto:contributors@expensify.com).

## Asking Questions
The best way to ask questions is to join our #expensify-contributors Slack channel. To request an invite to the channel, just email contributors@expensify.com with the subject "Slack Channel Invite" and we'll send you an invite! Please do not create issues to ask questions.
If you have any general questions, please ask in the #expensify-contributors Slack channel. To request an invite to the channel, just email contributors@expensify.com with the subject `Slack Channel Invite` and we'll send you an invite! The Expensify team will not be able to respond to direct messages in Slack.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still clarify that GH issues shouldn't be created for the sole purpose of asking questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I don't agree because I think we start opening the path to listing all ways to not ask a question if we do this type of approach. Right now I think it's really clear how/when to ask questions, and if we see someone violate that rule we just treat it like our manifesto (point them to the Contributing.md or Readme.md files, and ask them to re-read it).

I feel like listing it out is too exhaustive and specific - like we wouldn't want to get caught up saying "don't ask questions by... making new GH issues, messaging us in Upwork, emailing contributors@expensify.com, posting on the community, reaching out to concierge, etc"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we try out this suggestion, and circle back to update this copy if it's still generating issues to make the update you are proposing? I'm really hoping to keep brevity here so people actually read this guide 🤞 (but hey, time will tell if these edits actually help lol)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that works! We originally included it there since it is common for GH issues to be used to ask questions in open-source repositories, but I agree we can always re-add it if it actually becomes a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it is common for GH issues to be used to ask questions in open-source repositories

This argument is the first one I'll remember if contributors continue to do this in the future (I've never been an external contributor of another project so I didn't know this!). Let me know if our contributors keep doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably fine to take out, but I agree that we don't want people creating GH issues to ask questions.

CONTRIBUTING.md Outdated
Upon submission of a PR, please include a numbered list of explicit testing steps for each platform (Web, Desktop, iOS, and Android) to confirm the fix works as expected and there are no regressions.

#### Review Tips
9. Open a pull request, and make sure to fill in the required fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should re-add the GH link for creating a pull request from a fork here so it's clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a candid mistake on my end, adding it now and resolving this one.

@@ -38,23 +67,16 @@ We are currently managing payment via Upwork. If you'd like to be paid for your
[gpg]
program = gpg
```
1. [Open a PR](https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request-from-a-fork). Be sure to fill in all the required information on the PR template.
1. An Expensify engineer will be automatically assigned to review your PR
1. You will need all checks to pass:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are still important to highlight, including the CLA link. Is there a reason we removed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oversight on my part - as I thought we captured the part that was important, and didn't deem CLA stuff as fitting the bill. Apologies 😄, fixing now

Copy link
Contributor

@Jag96 Jag96 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! Will leave open for @marcaaron to review

@michaelhaxhiu
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA.

@michaelhaxhiu
Copy link
Contributor Author

recheck


#### Begin coding your solution in a pull reuqest
7. When you are ready to start, fork the repository and create a new branch.
8. Before you begin writing any code, please be aware that we require all commits to be [signed](https://docs.github.com/en/github/authenticating-to-github/signing-commits). The easiest way to do that is to [generate a new GPG key](https://docs.github.com/en/github/authenticating-to-github/generating-a-new-gpg-key) and [add it to your Github account](https://docs.github.com/en/github/authenticating-to-github/adding-a-new-gpg-key-to-your-github-account). Once you've done that, you can automatically sign all your commits by adding the following to your `.gitconfig`:
Copy link
Contributor

Choose a reason for hiding this comment

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

We lost the emphasis here. This step gets missed a lot 🤷

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM.

@marcaaron marcaaron merged commit e436274 into Hax-contributor-updates Mar 19, 2021
@marcaaron marcaaron deleted the michaelhaxhiu-contributor-patch-1 branch March 19, 2021 16:41
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants