Draft of Glowstone contributing & PR handling guidelines

CONTRIBUTING.md

Contributing to Glowstone

Glowstone is a lightweight, open source Minecraft server written in Java. For those who wish to contribute, we encourage you to fork the repository and submit pull requests. Below you will find guidelines that will explain this process in further detail.

Quick Guide

1. Create or find an issue on the issue tracker.
2. Fork Glowstone if you haven't done so already.
3. Create a branch dedicated to your change.
4. Write code addressing your feature or bug.
5. Commit your change according to the committing guidelines.
6. Push your branch and submit a pull request.

Getting Started

• Find an issue to fix or a feature to add.
• Search the issue tracker for your bug report or feature.
• Large changes should always have a separate issue to allow discussion.
  • If your feature or bug incorporates a large change, file a new issue, so the feature and its implementation may be tracked separately. This way, the nature of the issue may be discussed before time is spent addressing the issue.
• Fork the repository on GitHub.

Making Changes

• Create a topic branch from where you want to base your work.
  • This is usually the master branch.
  • Name your branch something relevant to the change you are going to make.
  • To quickly create a topic branch based on master, use git checkout master followed by git checkout -b <name>. Avoid working directly on the master branch.
• Make your best effort to meet our code style guidelines.
• Changes should generally be future-proof and not strongly tied to details that may change in a future Minecraft version.
• Large changes should be documented by the appropriate javadocs if applicable.

Committing your changes

• Check for unnecessary whitespace with git diff --check before committing.
• Describe your changes in the commit description.
• For a prolonged description, continue on a new line.
• If your change addresses an open issue, include in the first line one of:
  • For a bug-related issue: "Fixes #NNN".
  • For a feature request: "Resolves #NNN".

Example commit message:

Changed wgen to treat 128 as world height in all cases (fixes #151).

Submitting Your Changes

• Push your changes to the topic branch in your fork of the repository.
• Submit a pull request to this repository.
  • Be concise and to the point with your pull request title.
  • Explain in detail what your changes are and the motives behind them.
  • If your PR is not finished, but you are looking for review anyway, prefix title with [WIP].
• Await peer review and feedback.
• Revise PR based on feedback.

How to get your pull request accepted

• Ensure your change does not solely consist of formatting changes.
• Be concise and to the point in your pull request title.
• Address your changes in detail. Explain why you made each change.
• The code must be your work or you must appropriately credit those whose work you have used.

Additional Resources

• Bug tracker
• General GitHub documentation
• GitHub pull request documentation
• IRC: #glowstone (general) and #glowstonedev (development) on EsperNet

PR Handling.md

See PR Handling.

How the process sort of should go:

1. Contributor submits a PR (or, developer starts PR from feature branch). Ideally, there is one commit at this point, but this is not mandatory.
2. If the PR isn't complete, but early feedback is desired, precede title with [WIP].
3. Feedbackt on the PR takes place by PR handlers, maintainers (i.e. Space), and community devs.
4. More commits are pushed to the PR as work is done. Rebasing to master occurs at submitter's discretion.
5. Eventually, submitter & handler/community seem to agree PR is complete.
6. PR is rebased to master and squashed to one commit with a descriptive commit message.
7. The commit is merged (fast-forward or cherry-pick only - merge commits are evil).

General thoughts:

• PRs which implement some discrete chunk of a feature (e.g. Cows) but not the whole feature (e.g. all Animals) are acceptable.
• PRs which are intended to improve performance should only be accepted if there is evidence they succeed at doing so.
• PRs which fix bugs are acceptable.
• PRs which only change whitespace or styling are usually not accepted.
• If PR submitter disappears for too long (how long?) PR is closed. It may be reopened by submitter.
• If a PR is determined to be making a change which is not desirable, and the submitter does not wish to fix, it is closed.

• How many spaces?
• Mid-statement linebreaks?
• CRLF or LF (barely even a question)?
• If submitter makes no update within a week (maybe two), close.

Usually it's 4 spaces.

To amend @jkcclemens :

If submitter makes no update within a week (maybe two), close.

An "update" would be considered a response at the least or a commit. Something to prove they aren't dead. (Of course promises like "I'll get to this in a week" will be followed up on).

Questions:

• Are we using the GitHub merge button (or similar), or manually pulling the commit(s)?
• Do authors rebase/squash, or does the handler/puller?
• Is the handler the person that will pull/close?
• How is credit applied to multiple authors? (ie: someone submits a PR to the branch that has the open PR)
• How many handlers are to handle a single PR? Should the PR be assigned to the handler to indicate who is dealing with it? (if only one handler, I would say other handlers can still comment/review, but they would kinda be treated as part of the community when dealing with that PR. Although their feedback is also valuable and can help ensure that no outstanding issues were missed)

For general thoughts:

• Someone "above" the handler in the food chain should likely give a final answer on "yes" or "no". This leaves the PR handler to get the PR in tip-top shape as per whichever wishes are applicable.
• Community feedback is incredibly valuable, but shouldn't be accepted at face value (otherwise PRs/the project may become full of crap)
• I would say closing PRs that are badly formatted or clearly have no intention of reading the guidelines can be closed after (or with) a polite warning.
• Some kind of organization needs to be put in place. GitHub does a great job of listing everything with some basic filtering, but it does not provide the ability to manage and maintain information regarding the PR. That information could be private ideas or criticisms regarding the PR content or otherwise information that may not be ideal to be placed face value on the page.

That's just a dump of stuff I noticed while reading though. There may be more (or less) that actually should be looked at.

Squashing commits?
I guess you are following svn/cvs commit rules (linear) instead of git ones (merge pulls)?

Squashing is the act of taking 30 commits and making them one.

Yes, linear commits. There are advantages to both ways but I think linear will keep things cleaner and more sane on the project going forward. Glowstone is a pretty fast-moving project so I think a week expiry on dead PRs is sufficient. If the submitter continues work, they can reopen it.

Are we using the GitHub merge button (or similar), or manually pulling the commit(s)?

Manually pulling, I think. I'll spend some time seeing if the quick merge button can be made to work satisfactorily.

Do authors rebase/squash, or does the handler/puller?

Authors, should include the descriptive commit message in this.

Is the handler the person that will pull/close?

As per your suggestion futher down, handler passes off to maintainer for final approval and to perform the merge & close.

How is credit applied to multiple authors? (ie: someone submits a PR to the branch that has the open PR)

Primary author (PR submitter) has commit authorship, commit message should clearly thank (@ mention) anyone else who directly submitted code.

How many handlers are to handle a single PR? Should the PR be assigned to the handler to indicate who is dealing with it? (if only one handler, I would say other handlers can still comment/review, but they would kinda be treated as part of the community when dealing with that PR. Although their feedback is also valuable and can help ensure that no outstanding issues were missed)

I like the idea of one handler each PR is assigned to (assigned how? self? randomly/alternating? by higher up?) who is responsible for the boring stuff, and other handlers considered as part of the community.

Someone "above" the handler in the food chain should likely give a final answer on "yes" or "no". This leaves the PR handler to get the PR in tip-top shape as per whichever wishes are applicable.

Makes sense.

Community feedback is incredibly valuable, but shouldn't be accepted at face value (otherwise PRs/the project may become full of crap)

Makes sense. Handler on the PR responsible for sorting through the cruft?

I would say closing PRs that are badly formatted or clearly have no intention of reading the guidelines can be closed after (or with) a polite warning.

Polite warning, give submitter some time (a few days?) to fix formatting before closure (if they fix it later than that, they can reopen).

Some kind of organization needs to be put in place. GitHub does a great job of listing everything with some basic filtering, but it does not provide the ability to manage and maintain information regarding the PR. That information could be private ideas or criticisms regarding the PR content or otherwise information that may not be ideal to be placed face value on the page.

Private or less-public-than-usual communication channels (like the dev IRC) can be considered for some things, but I like to err on the side of openness.

How are we on line length? Oracle standard says 80 but there is existing code beyond 80 at the moment.

I'm liking everything mentioned here. I personally prefer people to keep a single commit in their PRs, --forcing as necessary. Any commits after comments are left. Just my 2c. Another point is be sure that commit messages are professional and aren't nonsensical.