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

[MRG] Add "good PR" examples #1848

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
27 changes: 26 additions & 1 deletion doc/dev/getting-started.rst
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ Congratulations! You're ready to develop!
Claiming an issue and starting to develop
-----------------------------------------

While consulting these instructions, you may find it helpful to refer to the
"Example pull requests" section below to see the workflow described here in
action.

#. Find an open issue and claim it.

Go to `the list of open khmer issues <https://github.com/dib-lab/khmer/issues?direction=desc&sort=created&state=open>`__ and find one you like; we suggest starting with `the low-hanging fruit issues <https://github.com/dib-lab/khmer/issues?direction=desc&labels=low-hanging-fruit&page=1&sort=created&state=open>`__).
Expand Down Expand Up @@ -245,7 +249,8 @@ Claiming an issue and starting to develop

git push origin

#. When you are ready to have the pull request reviewed, please mention @luizirber, @camillescott, @standage, @betatim, and/or @ctb with the comment 'Ready for review!'
#. When you are ready to have the pull request reviewed, post a comment with the text 'Ready for review!'
If desired, you can suggest specific developers or maintainers for review using an @ followed directly by their GitHub username.

#. The khmer team will now review your pull request and communicate with you through the pull request page.
Please feel free to add 'ping!' and an @ in the comments if you are looking for feedback—this will alert us that you are still on the line.
Expand All @@ -260,6 +265,26 @@ Congratulations on making your first contribution to the khmer library!
You're now an experienced GitHub user and an official khmer contributor!


Example pull requests
---------------------

If you are new to git or GitHub, you may find it helpful to see our pull request workflow in action.
Here are a few examples.

- `Thread #1732 <https://github.com/dib-lab/khmer/pull/1732>`_ is a very minimal pull request.
The first post includes a brief description of the small changes that were made, and indicated the changes were ready for peer review.
Following the initial post, the thread includes no discussion and is comprised almost entirely of automated messages generated by GitHub.

- `Thread #1791 <https://github.com/dib-lab/khmer/pull/1791>`_ likewise has very small changes to the code, but the thread includes a bit more discussion than the previous example.
The initial post includes some performance measurements, and subsequent comments include discussions of these numbers as well as requests for clarification on the code.
The author didn't explicitly request peer review of the code until the next day, but several people had already done an informal review of the code at that point.

- `Thread #1792 <https://github.com/dib-lab/khmer/pull/1792>`_ is a much more extensive pull request.
This PR addressed a core functionality in khmer, and accordingly the code changes were far reaching.
Over a month passed between when the PR was created and when the author requested final code review.
Along the way, however, the author requested feedback from other contributors, posted preliminary results, and posted many commits to the thread.


After your first issue is successfully merged...
------------------------------------------------

Expand Down
65 changes: 0 additions & 65 deletions doc/dev/guidelines-continued-dev.rst
Original file line number Diff line number Diff line change
Expand Up @@ -200,38 +200,6 @@ After this you'll have to add and commit the merge just like any other set
of changes. It's also recommended that you run tests.


Virtual environments
--------------------

The khmer package, like many software packages, relies on other third-party
software. Some of this software has been bundled together with khmer and is
compiled when you invoke ``make`` on the command line. But some of the software
khmer depends on is distributed as Python packages separately from khmer.

Python `virtual environments <https://pypi.python.org/pypi/virtualenv>`_ were
designed to isolate a stable development environment for a particular project.
This makes it possible to maintain different versions of a Python package for
different projects on your computer.

The installation instructions in the :doc:`Getting Started <getting-started>`
docs install the ``virtualenv`` command on your computer. After completing those
instructions, you can create a virtual environment with the command::

virtualenv -p python2 env/

(You can substitute `python3` for `python2` if Python version 3 is installed on
your system.) This command will create a new directory `env/` containing your
new virtual environment. The command::

source env/bin/activate

will activate the virtual environment. Now any Python packages that you install
with ``pip`` or ``make install-dep`` will be installed into your isolated
virtual environment.

Note that any time you create a new terminal session, using the virtual
environment requires that you re-activate it.

Pull request cleanup (commit squashing)
---------------------------------------

Expand Down Expand Up @@ -261,36 +229,3 @@ See also `Code reviews: the lab meeting for code
<http://fperez.org/py4science/code_reviews.html>`__ and
`the PyCogent coding guidelines
<http://pycogent.org/coding_guidelines.html>`__.

CPython Checklist
-----------------

Here's a checklist for new CPython types with future-proofing for Python 3::

- [ ] the CPython object name is of the form `khmer_${OBJECTNAME}_Object`
- [ ] Named struct with `PyObject_HEAD` macro
- [ ] `static PyTypeObject khmer_${OBJECTNAME}_Type` with the following
entries
- [ ] `PyVarObject_HEAD_INIT(NULL, 0)` as the object init (this includes
the `ob_size` field).
- [ ] all fields should have their name in a comment for readability
- [ ] The `tp_name` filed is a dotted name with both the module name and
the name of the type within the module. Example: `khmer.ReadAligner`
- [ ] Deallocator defined and cast to `(destructor)` in tp_dealloc
- [ ] The object's deallocator must be
`Py_TYPE(obj)->tp_free((PyObject*)obj);`
- [ ] Do _not_ define a `tp_getattr`
- [ ] BONUS: write methods to present the state of the object via
`tp_str` & `tp_repr`
- [ ] _Do_ pass in the array of methods in `tp_methods`
- [ ] _Do_ define a new method in `tp_new`
- [ ] PyMethodDef arrays contain doc strings
- [ ] Methods are cast to `PyCFunctions`s
- [ ] Type methods use their type Object in the method signature.
- [ ] Type creation method decrements the reference to self
(`Py_DECREF(self);`) before each error-path exit (`return NULL;`)
- [ ] No factory methods. Example: `khmer_new_readaligner`
- [ ] Type object is passed to `PyType_Ready` and its return code is checked
in `MOD_INIT()`
- [ ] The reference count for the type object is incremented before adding
it to the module: `Py_INCREF(&khmer_${OBJECTNAME}_Type);`.