Refresh the contributing file #4596

Merged
merged 1 commit into from Mar 10, 2016

Conversation

Projects
None yet
6 participants
@benbalter
Contributor

benbalter commented Feb 26, 2016

This pull request contains a rewrite of the CONTRIBUTING.markdown file. Reading through the file, I felt that it was written for maintainers, not for potential contributors, that it tended to police contributions, more than encourage them.

Rendered

To that end, this pull request does a few things:

  • Explicitly encourage contributions, and make things a bit less intimidating for first-time contributors
  • Create a new "where to get help or report a problem" section right up front, to help disambiguate the various Jekyll forums and resources
  • Describe different ways to contribute, both technical and non-technical
  • Explain the web-based pull request flow
  • Generally try to make the file flow from least-technical to most technical, putting how before "you must" or specific implementation details
  • Symlinked /CONTRIBUTING.markdown to site/_docs/contributing.md (we could theoretically do this in reverse as well) to keep the two in sync.
CONTRIBUTING.markdown
+layout: docs
+title: Contributing
+permalink: /docs/contributing/
+---

This comment has been minimized.

This comment has been minimized.

@benbalter

benbalter Feb 27, 2016

Contributor

Done via #4601.

@benbalter

benbalter Feb 27, 2016

Contributor

Done via #4601.

CONTRIBUTING.markdown
-If you are only updating a file in `test/`, you can use the command:
+* If you have a question about using Jekyll, start a discussion on https://talk.jekyllrb.com.
+* If you think you've found a bug within Jekyll itself, [open an issue](https://github.com/jekyll/jekyll/issues/new)

This comment has been minimized.

@parkr

parkr Feb 26, 2016

Member

Should we discuss issues with plugins?

@parkr

parkr Feb 26, 2016

Member

Should we discuss issues with plugins?

CONTRIBUTING.markdown
-If you are only updating a `.feature` file, you can use the command:
+Whether you're a developer, a designer, or just a Jekyll devotee, there are lots of ways to contribute. Here's a few ideas:

This comment has been minimized.

@parkr

parkr Feb 26, 2016

Member

I'm a developer AND a Jekyll devotee! What does it mean to be a devotee? 😄

@parkr

parkr Feb 26, 2016

Member

I'm a developer AND a Jekyll devotee! What does it mean to be a devotee? 😄

This comment has been minimized.

@chrisfinazzo

chrisfinazzo Feb 27, 2016

Contributor

Well, you're steering the ship. So, how about...Grand Poobah™? 😝

@chrisfinazzo

chrisfinazzo Feb 27, 2016

Contributor

Well, you're steering the ship. So, how about...Grand Poobah™? 😝

This comment has been minimized.

@benbalter

benbalter Feb 27, 2016

Contributor

Was looking for a third "D" word that would mean someone who would want to contribute, but isn't a developer or a designer. Suggestions welcome. 😄

@benbalter

benbalter Feb 27, 2016

Contributor

Was looking for a third "D" word that would mean someone who would want to contribute, but isn't a developer or a designer. Suggestions welcome. 😄

CONTRIBUTING.markdown
+
+## Proposing updates to the documentation
+
+We want the Jekyll documentation to be the best it can be. We've open-sourced our docs and we welcome any pull requests if you find it lacking.

This comment has been minimized.

@parkr

parkr Feb 26, 2016

Member

open-sourced our docs could link to github.com/jekyll/jekyll/tree/master/site?

@parkr

parkr Feb 26, 2016

Member

open-sourced our docs could link to github.com/jekyll/jekyll/tree/master/site?

This comment has been minimized.

@parkr

parkr Feb 26, 2016

Member

Oh, you link below!

@parkr

parkr Feb 26, 2016

Member

Oh, you link below!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 26, 2016

Contributor

Can this be rebased to remove that hard merge?

Contributor

envygeeks commented Feb 26, 2016

Can this be rebased to remove that hard merge?

CONTRIBUTING.markdown
+
+That's it! You'll be automatically subscribed to receive updates as others review your proposed change and provide feedback.
+
+### Submitting a pull request via Git command line

This comment has been minimized.

@chrisfinazzo

chrisfinazzo Feb 27, 2016

Contributor

@benbalter, I raised this question on Jekyll Talk and @parkr directed me here, so I think it merits at least some consideration...

Do you think it is worthwhile to ask people to rebase master when they submit PR's here? In a few instances, I have actually abandoned a request, copied my work elsewhere and checked out a new branch just to ensure that I was working off the latest commit. Granted, some of this may come from not being entirely comfortable with the concept yet, and it wouldn't work in all cases, but it might make things easier for the Core team.

@chrisfinazzo

chrisfinazzo Feb 27, 2016

Contributor

@benbalter, I raised this question on Jekyll Talk and @parkr directed me here, so I think it merits at least some consideration...

Do you think it is worthwhile to ask people to rebase master when they submit PR's here? In a few instances, I have actually abandoned a request, copied my work elsewhere and checked out a new branch just to ensure that I was working off the latest commit. Granted, some of this may come from not being entirely comfortable with the concept yet, and it wouldn't work in all cases, but it might make things easier for the Core team.

This comment has been minimized.

@benbalter

benbalter Feb 27, 2016

Contributor

just to ensure that I was working off the latest commit

If we activate protected branches, we'll get a shiny "update branch" button, which auto merges in master (not to mention, Git is smart enough to do the reverse in most cases).

I'm of the opinion that there are more important things to care about than obsessing over a pristine Git history. In practice, that means it's better to have a PR with a merge commit, than not to have a PR at all because we've required the contributor (who may be submitting their first pull request) to install Git command line and learn advanced concepts like rebasing before their contributing could be considered. Practicality over purity.

@benbalter

benbalter Feb 27, 2016

Contributor

just to ensure that I was working off the latest commit

If we activate protected branches, we'll get a shiny "update branch" button, which auto merges in master (not to mention, Git is smart enough to do the reverse in most cases).

I'm of the opinion that there are more important things to care about than obsessing over a pristine Git history. In practice, that means it's better to have a PR with a merge commit, than not to have a PR at all because we've required the contributor (who may be submitting their first pull request) to install Git command line and learn advanced concepts like rebasing before their contributing could be considered. Practicality over purity.

This comment has been minimized.

@chrisfinazzo

chrisfinazzo Feb 28, 2016

Contributor

I see. If there'a a way to do this within GitHub's flow, that's probably good enough for most people.

@chrisfinazzo

chrisfinazzo Feb 28, 2016

Contributor

I see. If there'a a way to do this within GitHub's flow, that's probably good enough for most people.

This comment has been minimized.

@envygeeks

envygeeks Feb 28, 2016

Contributor

I believe protected branches are enabled for this repo and have been since the early days of certain repositories getting it, because I remember accidentally trying to force push to jekyll/jekyll and being rejected.

@envygeeks

envygeeks Feb 28, 2016

Contributor

I believe protected branches are enabled for this repo and have been since the early days of certain repositories getting it, because I remember accidentally trying to force push to jekyll/jekyll and being rejected.

This comment has been minimized.

@benbalter

benbalter Feb 28, 2016

Contributor

Sorry. Not protected branches. Required status checks.

@benbalter

benbalter Feb 28, 2016

Contributor

Sorry. Not protected branches. Required status checks.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Feb 27, 2016

Contributor

Symlinks are not supported on Windows, this is one of those so-called barriers.

Removed the need for a symlink via #4601.

Contributor

benbalter commented Feb 27, 2016

Symlinks are not supported on Windows, this is one of those so-called barriers.

Removed the need for a symlink via #4601.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 5, 2016

Contributor

Merged in master now that #4601 has been merged.

Contributor

benbalter commented Mar 5, 2016

Merged in master now that #4601 has been merged.

.github/CONTRIBUTING.markdown
-If you are only updating a `.feature` file, you can use the command:
+* [Install Jekyll on your computer](https://jekyllrb.com/docs/installation/) and kick the tires. Does it work? Does it do what you'd expect? If not, [open an issue](https://github.com/jekyll/jekyll/issues/new) and let us know.
+* Comment on some of the project's [open issue](https://github.com/jekyll/jekyll/issues). Have you experienced the same problem? Know a work around? Do you have a suggestion for how the feature could be better?

This comment has been minimized.

@mattr-

mattr- Mar 8, 2016

Member

Could you make "issue" plural in the link? IMO, "project's open issues" reads better in the context of that sentence.

@mattr-

mattr- Mar 8, 2016

Member

Could you make "issue" plural in the link? IMO, "project's open issues" reads better in the context of that sentence.

.github/CONTRIBUTING.markdown
-Updating Documentation
-----------------------
+Interesting in submitting a pull request? Awesome. Read on. There's a few common gotchas, we'd love to help you avoid.

This comment has been minimized.

@mattr-

mattr- Mar 8, 2016

Member

I don't believe you need a comma in the sentence There's a few common gotchas, we'd love to help you avoid. I know of two different ways to improve this sentence.

  1. Remove the comma 😃

or

  1. Split the sentence: There's a few common gotchas. We'd love to help you avoid them.
@mattr-

mattr- Mar 8, 2016

Member

I don't believe you need a comma in the sentence There's a few common gotchas, we'd love to help you avoid. I know of two different ways to improve this sentence.

  1. Remove the comma 😃

or

  1. Split the sentence: There's a few common gotchas. We'd love to help you avoid them.
@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 8, 2016

Member

❤️ Thank you for the updates! This reads much better. Just a few comments and if there's no other objections from @jekyll/core, we should get this merged in.

Member

mattr- commented Mar 8, 2016

❤️ Thank you for the updates! This reads much better. Just a few comments and if there's no other objections from @jekyll/core, we should get this merged in.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 9, 2016

Contributor

@mattr- stupid mistakes on my part both. Thanks for the 👀. Updated with your feedback.

Contributor

benbalter commented Mar 9, 2016

@mattr- stupid mistakes on my part both. Thanks for the 👀. Updated with your feedback.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 9, 2016

Member

LGTM.

Member

parkr commented Mar 9, 2016

LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 9, 2016

Contributor

@benbalter can we get this rebased as a single (or series of logical) commits? It's hard for me to keep up with what's going on with all these commits going down.

Contributor

envygeeks commented Mar 9, 2016

@benbalter can we get this rebased as a single (or series of logical) commits? It's hard for me to keep up with what's going on with all these commits going down.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 9, 2016

Contributor

@envygeeks tried rebasing, even down to a single commit, but kept running into at least three complex merge conflicts, regardless of what I did, as this relies on changes now on master. If you're able to rebase this branch, awesome, free free to push directly, otherwise, I'm going to suggest we merge it as is. 😄

Contributor

benbalter commented Mar 9, 2016

@envygeeks tried rebasing, even down to a single commit, but kept running into at least three complex merge conflicts, regardless of what I did, as this relies on changes now on master. If you're able to rebase this branch, awesome, free free to push directly, otherwise, I'm going to suggest we merge it as is. 😄

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 9, 2016

Contributor

@benbalter I'll rebase it.

Contributor

envygeeks commented Mar 9, 2016

@benbalter I'll rebase it.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 10, 2016

Member

rebased!

@jekyllbot: merge +dev

Member

parkr commented Mar 10, 2016

rebased!

@jekyllbot: merge +dev

jekyllbot added a commit that referenced this pull request Mar 10, 2016

@jekyllbot jekyllbot merged commit a068bfb into master Mar 10, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jekyllbot jekyllbot deleted the contributing-refresh branch Mar 10, 2016

jekyllbot added a commit that referenced this pull request Mar 10, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
Contributor

benbalter commented Mar 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment