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

Documentation about contributing #227

Merged
merged 21 commits into from
Jan 29, 2018
Merged
Show file tree
Hide file tree
Changes from 5 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
5 changes: 5 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Contributing
============

Please see the [project documentation](http://zarr.readthedocs.io/en/stable/contributing.html) for information about contributing to Zarr.

200 changes: 200 additions & 0 deletions docs/contributing.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
Contributing to Zarr
====================

Zarr is a community maintained project. We welcome contributions in the form of bug
reports, bug fixes, documentation, enhancement proposals and more. This page provides
information on how best to contribute.

Asking for help
---------------

If you have a question about how to use Zarr, please post your question on
StackOverflow using the `"zarr" tag <https://stackoverflow.com/questions/tagged/zarr>`_
. If you don't get a response within a day or two, feel free to raise a `GitHub issue
<https://github.com/alimanfoo/zarr/issues/new>`_ including a link to your StackOverflow
question. We will try to respond to questions as quickly as possible, but please bear
in mind that there may be periods where we have limited time to answer questions
due to other commitments.
Copy link
Member

Choose a reason for hiding this comment

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

👍


Bug reports
-----------

If you find a bug, please raise a `GitHub issue
<https://github.com/alimanfoo/zarr/issues/new>`_. Please include the following items in
a bug report:
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely good. Should we link over to the issue template for more details?


1. A minimal, self-contained snippet of Python code reproducing the problem. You can
format the code nicely using markdown, e.g.::


```python
>>> import zarr
>>> g = zarr.group()
...
```

2. Information about the version of Zarr, along with versions of dependencies and the
Python interpreter, and installation information. The version of Zarr can be obtained
from the ``zarr.__version__`` property. Please also state how Zarr was installed,
e.g., "installed via pip into a virtual environment", or "installed using conda".
Information about other packages installed can be obtained by executing ``pip list``
(if using pip to install packages) or ``conda list`` (if using conda to install
packages) from the operating system command prompt. The version of the Python
interpreter can be obtained by running a Python interactive session, e.g.::

$ python
Python 3.6.1 (default, Mar 22 2017, 06:17:05)
[GCC 6.3.0 20170321] on linux

3. An explanation of why the current behaviour is wrong/not desired, and what you
expect instead.

Enhancement proposals
---------------------

If you have an idea about a new feature or some other improvement to Zarr, please raise a
`GitHub issue <https://github.com/alimanfoo/zarr/issues/new>`_ first to discuss.

We very much welcome ideas and suggestions for how to improve Zarr, but please bear in
mind that we are likely to be conservative in accepting proposals for new features. The
reasons for this are that we would like to keep the Zarr code base lean and focused on
a core set of functionalities, and available time for development, review and maintenance
of new features is limited. But if you have a great idea, please don't let that stop
you posting it on GitHub, just please don't be offended if we respond cautiously.
Copy link
Member

Choose a reason for hiding this comment

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

you posting -> you from posting

Copy link
Member Author

Choose a reason for hiding this comment

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

done


Contributing code and/or documentation
--------------------------------------

Forking the repository
~~~~~~~~~~~~~~~~~~~~~~

The Zarr source code is hosted on GitHub at the following location:

* `https://github.com/zarr-developers/zarr <https://github.com/zarr-developers/zarr>`_

You will need your own fork to work on the code. Go to the link above and hit
the "Fork" button. Then clone your fork to your local machine::

$ git clone [email protected]:your-user-name/zarr.git
$ cd zarr
$ git remote add upstream [email protected]:zarr-developers/zarr.git

Creating a development environment
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To work with the Zarr source code, it is recommended to set up a Python virtual
environment and install all Zarr dependencies using the same versions as are used by
the core developers and continuous integration services. Assuming you have a Python
3 interpreter already installed, and have also installed the virtualenv package, and
you have cloned the Zarr source code and your current working directory is the root of
the repository, you can do something like the following::

$ mkdir -p ~/pyenv/zarr-dev
$ virtualenv --no-site-packages --python=/usr/bin/python3.6 ~/pyenv/zarr-dev
$ source ~/pyenv/zarr-dev/bin/activate
$ pip install -r requirements_dev.txt
$ pip install -r requirements_dev_optional.txt
$ pip install -e .

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest also including an example of how to do this with conda. My experience is that this is a common workflow.

$ conda env create -n zarr-dev -f requirements_dev.yml  # req file would need to be added
$ source activate zarr-dev
$ pip install -e  # or python setup.py develop

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't give a conda example as (last time I looked) you can't use tox with conda, and tox is the best way at the moment to run all the tests (including doctests, coverage, flake8 etc.) and verify support for PY2 and PY3.

Copy link
Member

Choose a reason for hiding this comment

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

virtualenv has been packaged and exists on defaults and conda-forge. Once that was done tox was added to conda-forge in short order.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll leave this as-is for now as I don't have time to verify all the steps to get tox fully up and running all test environments under a conda installation, but if someone wants to do this later and can provide the necessary commands then very happy to add.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. That was only meant as an informative comment. Happy with this as is.

To verify that your development environment is working, you can run the unit tests::

$ pytest -v zarr

Creating a branch
~~~~~~~~~~~~~~~~~

It's best to create a new, separate branch for each piece of work you want to do. E.g.::

git checkout -b shiny-new-feature
Copy link
Member

Choose a reason for hiding this comment

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

I would preface this with:

git fetch upstream
git checkout -b shiny-new-feature upsteam/master

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will add.


This changes your working directory to the shiny-new-feature branch. Keep any changes in
this branch specific to one bug or feature so it is clear what the branch brings to
Zarr.

To update this branch with latest code from Zarr, you can retrieve the changes from
the master branch::

git fetch upstream
git rebase upstream/master
Copy link
Member

Choose a reason for hiding this comment

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

While I love rebasing personally, working with developers from various skill levels has caused me to suggest merging to more general audiences. Rebasing can mean trying to resolve many conflicts in many steps instead of all conflicts in one step. The latter seems to avoid many new issues cropping up and allows any new issues to be resolved in subsequent commits pretty easily. Not to mention merging tends to be friendlier if other people help out on your branch.

These are all personal opinions and observations over time. Feel free to ignore them if you have a strong preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a fair point, I've seen people struggle with rebase, I remember the first time I tried it I went round in circles for about an hour before someone told me you had to do git push -f at the end.


This will replay your commits on top of the latest Zarr git master. If this leads to
merge conflicts, these need to be resolved before submitting a pull request.

Before you do any new work or submit a pull request, please open an issue on GitHub to
report the bug or propose the feature you'd like to add.
Copy link
Member

Choose a reason for hiding this comment

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

👍 Maybe this should move up to the top of the section?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


Running the test suite
~~~~~~~~~~~~~~~~~~~~~~

Zarr includes a suite of unit tests, as well as doctests included in function and class
docstrings and in the tutorial and storage spec. The simplest way to run the unit tests
is to invoke::

$ pytest -v zarr

To also run the doctests within docstrings, run::

$ pytest -v --doctest-modules zarr

To run the doctests within the tutorial and storage spec, run::

$ python -m doctest -o NORMALIZE_WHITESPACE -o ELLIPSIS docs/tutorial.rst docs/spec/v2.rst

Tests can be run under different Python versions using tox. E.g. (assuming you have the
corresponding Python interpreters installed on your system)::

$ tox -e py27,py34,py35,py36

Zarr currently supports Python 2.7 and Python 3.4-3.6, so the above command must
succeed before code can be accepted into the main code base. Note that only the py36
tox environment runs the doctests, i.e., doctests only need to succeed under Python 3.6.

All tests are automatically run via Travis (Linux) and AppVeyor (Windows) continuous
integration services for every pull request. Tests must pass under both services before
code can be accepted.
Copy link
Member

@jakirkham jakirkham Jan 4, 2018

Choose a reason for hiding this comment

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

Should we make a note about coverage requirements here too? For example, "total coverage over all builds must 100% (individual builds may be lower due to Python 2/3 or other differences)."

Copy link
Member

Choose a reason for hiding this comment

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

Oops...should have kept reading. 😳 Though a one liner here would still be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


Code standards
~~~~~~~~~~~~~~

All code must conform to the PEP8 standard. Regarding line length, lines up to 100
characters are allowed, although please try to keep under 90 wherever possible.
Conformance can be checked by running::

$ flake8 --max-line-length=100 zarr

This is automatically run when invoking ``tox -e py36``.

Test coverage
~~~~~~~~~~~~~

Zarr maintains 100% test coverage under the latest Python stable release (currently
Python 3.6). Both unit tests and docstring doctests are included when computing
coverage. Running ``tox -e py36`` will automatically run the test suite with coverage
and produce a coverage report. This should be 100% before code can be accepted into the
main code base.

When submitting a pull request, coverage will also be collected across all supported
versions via the Coveralls service, and will be reported back within the pull request.
Coveralls coverage must also be 100% before code can be accepted.

Documentation
~~~~~~~~~~~~~

Docstrings for user-facing classes and functions should follow the `numpydoc
<https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt>`_ standard,
including sections for Parameters and Examples. All examples will be run as doctests
under Python 3.6.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Python 3.6 -> Python 3.6 only

Copy link
Member Author

Choose a reason for hiding this comment

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

done


Zarr uses Sphinx for documentation, hosted on readthedocs.org. Documentation is
written in the RestructuredText markup language (.rst files) in the ``docs`` folder.
The documentation consists both of prose and API documentation. All user-facing classes
and functions should be included in the API documentation, under the ``docs/api``
folder. Any new features or important usage information should be included in the
tutorial (``docs/tutorial.rst``). Any changes should also be included in the release
notes (``docs/release.rst``).

The documentation can be built by running::

$ tox -e docs

The resulting built documentation will be available in the ``.tox/docs/tmp/html`` folder.
2 changes: 2 additions & 0 deletions requirements_dev_optional.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bsddb3==6.2.5
lmdb==0.93
3 changes: 1 addition & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ commands =
deps =
-rrequirements_dev.txt
# linux only
bsddb3==6.2.5
lmdb==0.93
-rrequirements_dev_optional.txt


[testenv:docs]
Expand Down
5 changes: 5 additions & 0 deletions zarr/convenience.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,11 @@ def tree(grp, expand=False, level=None):
--------
zarr.hierarchy.Group.tree

Notes
-----
Please note that the behaviour of this function is still evolving and the
default output and/or parameters may change in future versions.
Copy link
Member

Choose a reason for hiding this comment

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

SGTM. 👍


"""

return TreeViewer(grp, expand=expand, level=level)
Expand Down
5 changes: 5 additions & 0 deletions zarr/hierarchy.py
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,11 @@ def tree(self, expand=False, level=None):
└── quux
└── baz (100,) float64

Notes
-----
Please note that the behaviour of this function is still evolving and the
default output and/or parameters may change in future versions.
Copy link
Member

Choose a reason for hiding this comment

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

SGTM. 👍


"""

return TreeViewer(self, expand=expand, level=level)
Expand Down
72 changes: 34 additions & 38 deletions zarr/tests/test_convenience.py
Original file line number Diff line number Diff line change
Expand Up @@ -579,42 +579,38 @@ def test_logging(self):
h5py = None


def temp_h5f():
fn = tempfile.mktemp()
atexit.register(os.remove, fn)
h5f = h5py.File(fn, mode='w')
atexit.register(lambda v: v.close(), h5f)
return h5f
if h5py is not None:


@pytest.mark.skipif(h5py is None, reason='h5py not installed')
Copy link
Member

Choose a reason for hiding this comment

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

we also ran into this in pydata/xarray#1557. This should work but just for reference, we created our own skipif machinery using unittest.

https://github.com/pydata/xarray/blob/186a8bbe6a79e8d94203af113297c96a045b8730/xarray/tests/__init__.py#L47-L61

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, very helpful.

class TestCopyHDF5ToZarr(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyHDF5ToZarr, self).__init__(*args, **kwargs)
self.source_h5py = True
self.dest_h5py = False
self.new_source = temp_h5f
self.new_dest = group


@pytest.mark.skipif(h5py is None, reason='h5py not installed')
class TestCopyZarrToHDF5(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyZarrToHDF5, self).__init__(*args, **kwargs)
self.source_h5py = False
self.dest_h5py = True
self.new_source = group
self.new_dest = temp_h5f


@pytest.mark.skipif(h5py is None, reason='h5py not installed')
class TestCopyHDF5ToHDF5(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyHDF5ToHDF5, self).__init__(*args, **kwargs)
self.source_h5py = True
self.dest_h5py = True
self.new_source = temp_h5f
self.new_dest = temp_h5f
def temp_h5f():
fn = tempfile.mktemp()
atexit.register(os.remove, fn)
h5f = h5py.File(fn, mode='w')
atexit.register(lambda v: v.close(), h5f)
return h5f

class TestCopyHDF5ToZarr(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyHDF5ToZarr, self).__init__(*args, **kwargs)
self.source_h5py = True
self.dest_h5py = False
self.new_source = temp_h5f
self.new_dest = group

class TestCopyZarrToHDF5(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyZarrToHDF5, self).__init__(*args, **kwargs)
self.source_h5py = False
self.dest_h5py = True
self.new_source = group
self.new_dest = temp_h5f

class TestCopyHDF5ToHDF5(TestCopy):

def __init__(self, *args, **kwargs):
super(TestCopyHDF5ToHDF5, self).__init__(*args, **kwargs)
self.source_h5py = True
self.dest_h5py = True
self.new_source = temp_h5f
self.new_dest = temp_h5f
Loading