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

chore(docs): Add contributing and pr/issue templates #21

Merged
merged 5 commits into from
Apr 18, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
30 changes: 30 additions & 0 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<!--- Provide a general summary of the issue in the Title above -->

## Current Behavior
<!--- If describing a bug, tell us what is broken -->
<!--- If suggesting a change/improvement tell us what is missing from the SDK as it is currently -->

## Expected Behavior
<!--- If describing a bug, tell us how the SDK should behave without the bug -->
<!--- If you're suggesting changes, outline your idea here -->

## Possible Solution
<!--- Optional, but helpful! Use this field to suggest bug solutions or change impementation details. Feel free to get technical. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the

Optional, but helpful!

part to the end? I really want folks to do this part 😄


## Steps to Reproduce (for bugs)
<!--- Provide a link to a live example, or an unambiguous set of steps to -->
<!--- reproduce this bug. Include code to reproduce, if relevant -->
* Link to your project or codepen:
1.
2.
3.
4.

## Screenshots (if appropriate):

## Your Environment
<!--- Please include any relevant environmental information here. (SDK version, node version, browser etc.) -->
* SDK version:
* Node version:
* Browser name and version:
* Operating system:
18 changes: 18 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!--- Provide a general summary of your changes in the Title above -->
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to mention that the PR needs to follow these commit guidelines: https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#-git-commit-guidelines. Specifically the title needs to <type>(<scope>): <subject>


## Description
<!--- Describe your changes in detail -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Description needs to also include BREAKING CHANGE: if it's not backwards compatible (i.e. major bump)

<!--- Don't forget to note any issues here with fixes #<issue number> -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put quotes around "fixes #<issue number>"


## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding:

For bug fixes, include regression unit tests that fail w/o the fix

For new features, include unit tests for the new functionality


## Screenshots (if appropriate):

## Checklist:
<!--- Please mark an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] I have read the **CONTRIBUTING** document.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you can link to the CONTRIBUTING.md file? Mainly for other folks looking at the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it need to be a relative path since this file is nested in .github/ ? is there a way to test these readmes out before merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've honestly never done a template so I don't know what it would ultimately be relative too. This is all new to me so I'm excited! 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Upside, if it doesn't work we can quickly push a change that fixes it. 😅

- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] I have run yarn validate to ensure that tests, typescript and linting are all in order.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one isn't really necessary since it gets run in Travis automatically for the PR

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 its still nice to have people run the tests though, might encourage more passing prs in our pr queue

69 changes: 69 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#Contributing

Thank you for your interest in contributing to Eventbrite's Javascript SDK!

###Table of Contents
* [Workflow](#workflow)
* [Setup](#setup)
* [Branches](#using-branches-to-submit-changes)
* [Keeping up to date](#keeping-your-local-repo-up-to-date)
* [Creating issues](#creating-issues)
* [Working on and submitting changes](#working-on-and-submitting-changes)
* [Steps to submit](#steps-to-submit)

## Workflow

### Setup

1. Fork the repository
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link "Fork the repository" to: https://help.github.com/articles/fork-a-repo/

2. Clone your new forked repository to your local computer
3. Set the Eventbrite repository as your branches upstream branch
`git remote add upstream https://github.com/eventbrite/eventbrite-sdk-javascript.git`
4. Navigate to the root directory of your newly cloned repository
(for mac `cd /path/to/eventbrite-sdk-javascript`)
5. `yarn install` to install local dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we need to add "install Yarn" to part of the setup?

By the way, in markdown, you can number each one like 1. and it'll create an ordered list for you. That way you can add/reorder without having to re-number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice tip on the list! i'll just make everything 1. I'll link docs to installing yarn


### Using branches to submit changes
To work on changes to the Eventbrite repository, create a new branch on your local repository. `git checkout -b <your-new-branch-name>`


### Keeping your local repo up to date
To ensure your branch never gets out of sync with Eventbrite's master, ensure that you have your upstream set properly (see the [Setup](#setup) step)

1. `git checkout master` (you may have to stash or commit your local changes if on a new branch)
Copy link
Contributor

Choose a reason for hiding this comment

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

2. `git pull upstream master`
3. `git checkout <your-new-branch-name>`
4. `git rebase master`
5. If you've stashed changes, unstash them now, otherwise your branch should now be up to date

Always try to keep your master 'clean' by only pulling changes directly from upstream into your master branch and rebasing those changes onto your working branch.

It is always a good idea to pull the upstream branch in to your master branch before creating a new feature branch to work from. This will minimize the chances of encountering merge conflicts.

## Creating Issues
Create issues to File bugs, changes, and proposals.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: File should probably be lowercase.


Before opening a new issue, please [search][issues] to see if there has been previous discussion about the same feature or issue. If so, please contribute to the discussion there.

If nothing is found, feel free to [open a new issue][issues] and fill out the issue template to the best of your ability.

## Working on and submitting changes
When starting on improvements or new features that are non-trivial, it is always a good idea to first discuss the changes you wish to implement by [opening a github issue][issues] before getting started.

If you've found a bug or feature you'd like to work on in our [github issue tracker][issues], please comment on the issue to let others know that you'd like to work on it.

While implementing fixes, please try to change as little code as possible. This helps speed up the review process and helps diminish the chance of additional bugs.

Please try to conform to the coding style of the code base.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how specific we want to be put, but either a link to the coding style or how they could verify that. ie yarn run lint && yarn run format

Copy link
Contributor

Choose a reason for hiding this comment

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

Code gets automatically formatted on commit, so they should be good there. And then linting runs in Travis as part of validate script. I actually don't think we're running prettier verification though. If someone bypasses prettier when committing they could actually put a PR up w/ unformatted code I believe...

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 much like validating the title, we can probably find something to validate the the format is proper. Something like eslint-config-prettier should do the job.


###Steps to submit:

1. Please ensure that your changes are fully covered by one or more unit test/s.
Copy link
Contributor

Choose a reason for hiding this comment

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

"test(s)"

Copy link
Contributor Author

@ryanwholey ryanwholey Apr 10, 2018

Choose a reason for hiding this comment

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

lol thank you, that looked weird to me

2. Check to make sure that your changes are documented properly (inline comments for interesting lines, READMEs, etc.)
3. Run `yarn validate` to ensure that all tests pass, the linter is satisfied and your changes are typescript compliant.
4. PR titles must be prefixed by the type of changes the PR contains followed by the scope of what the pr touches. Please use one of `feat, fix, docs, style, refactor, perf, test, chore` as the prefix. The scope is the the direct product your changes affect. Example: `chore(build): Add encrypted ssh key for semantic-release` because its a chore and it touches the build.
* For multiple scope items, you can comma separate 2 or 3 but if there are more than that please use a `*` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@kwelch kwelch Apr 9, 2018

Choose a reason for hiding this comment

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

Yes, I believe that would be helpful.

We should probably add badge[s] to the readme that can give some of this context quickly as someone is looking at the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a semantic-release badge, but nothing (that I could find) for the angular commit convention. That'd be awesome

5. Please use a [closing issue keyword](https://help.github.com/articles/closing-issues-using-keywords/) to indicate the issue that your fix addresses in the description section of the pull request template. Example: `fixes #32` to close issue #32


[issues]: https://github.com/eventbrite/eventbrite-sdk-javascript/issues
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Read more on [getting a token](https://www.eventbrite.com/developer/v3/api_overv

## Contributing

Coming soon...
Check out our contributing document [here](https://github.com/eventbrite/eventbrite-sdk-javascript/blob/master/CONTRIBUTING.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do:

Contributions are welcome! See Contributing Guidelines for more details.

Also you can just do CONTRIBUTING.md as the URL. You don't have to do the full URL


## Project philosophy

Expand Down