Reorganize the developer docs #1512

Merged
merged 4 commits into from Nov 20, 2012

Conversation

Projects
None yet
5 participants
Owner

mdboom commented Nov 16, 2012

This is a reorganization of the developer docs. The main thrust here is to add a "pull request checklist" which is intended to be a concise and easy-to-follow checklist of things to ensure before a pull request is merged. Some larger sections of coding_guide.rst have been separated out into their own chapters.

There are a lot of instances where we (myself included) have said "oops -- forgot to document this feature committed a while back" or "forgot to add a CHANGELOG entry" etc. I thought that having a place to refer to every time would be helpful, and should also help new contributors understand what's expected, and take some burden off of the seasoned developers to say "please add tests, documentation, etc." in the pull request review -- we now have something we can just point to.

I don't necessarily want to get into an elaborate discussion about coding standards -- and I've tried to keep this simple so as to not seem overly rigid and daunting -- but if there's anything that should go on this list that isn't there already, please mention it here.

Member

dmcdougall commented Nov 16, 2012

This is exactly what I needed when I started contributing. Thank you so much for taking your time to put this together.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Nov 16, 2012

doc/devel/coding_guide.rst
-* Keep the maintenance branches and master in sync where it makes sense.
+* No tabs (only space). No trailing whitespace.
@WeatherGod

WeatherGod Nov 16, 2012

Member

Should this be a main bullet, or a sub-bullet for the PEP8 bullet?

@mdboom

mdboom Nov 16, 2012

Owner

PEP8 doesn't address trailing whitespace. The "no tabs" rule is in PEP8, but I'd rather make it really prominent here.

@WeatherGod WeatherGod commented on the diff Nov 16, 2012

doc/devel/coding_guide.rst
- import numpy.ma as ma
-
-For matplotlib main module, use::
-
- import matplotlib as mpl
- mpl.rcParams['xtick.major.pad'] = 6
-
-For matplotlib modules (or any other modules), use::
-
- import matplotlib.cbook as cbook
+ import numpy as np
+ import numpy.ma as ma
+ import matplotlib as mpl
+ from matplotlib import pyplot as plt
+ import matplotlib.cbook as cbook
@WeatherGod

WeatherGod Nov 16, 2012

Member

import matplotlib.collections as mcol
import matplotlib.patches as mpatches

or whatever it is that is standard...

@mdboom

mdboom Nov 16, 2012

Owner

I'll add these. I've never been a huge fan of that, but it's sometimes required, and of course is used a lot throughout the codebase. We can remove them here if we clean that style up globally later.

Member

WeatherGod commented Nov 16, 2012

Might want to add to the bullet list a reference to the section on what to do if adding a new plotting function.

@NelleV NelleV commented on an outdated diff Nov 19, 2012

doc/devel/testing.rst
+
+
+Running tests by any means other than `matplotlib.test()`
+does not load the nose "knownfailureif" (Known failing tests) plugin,
+causing known-failing tests to fail for real.
+
+Writing a simple test
+---------------------
+
+Many elements of Matplotlib can be tested using standard tests. For
+example, here is a test from :mod:`matplotlib.tests.test_basic`::
+
+ from nose.tools import assert_equal
+
+ def test_simple():
+ '''very simple example test'''
@NelleV

NelleV Nov 19, 2012

Contributor

This should probably be a "proper" docstring, with """ instead of '''

Contributor

NelleV commented Nov 19, 2012

I didn't take the time to checkout the code and building the documentation, but it seems to me like a huge improvement on the documentation. Nice job !

Member

pelson commented Nov 19, 2012

The main thrust here is to add a "pull request checklist" which is intended to be a concise and easy-to-follow checklist of things to ensure before a pull request is merged.

We should add a contributing.md file to help with that (for those submitting pull requests). Info: https://github.com/blog/1184-contributing-guidelines

Owner

mdboom commented Nov 19, 2012

@pelson: Cool: I didn't know about that feature of github. I agree, this content should go in there. But maybe contributing should just link to this page on matplotlib.org? I'm not sure keeping this documentation in sync in multiple places is a good idea.

@pelson pelson commented on the diff Nov 19, 2012

doc/devel/testing.rst
+
+Writing a simple test
+---------------------
+
+Many elements of Matplotlib can be tested using standard tests. For
+example, here is a test from :mod:`matplotlib.tests.test_basic`::
+
+ from nose.tools import assert_equal
+
+ def test_simple():
+ '''very simple example test'''
+ assert_equal(1+1,2)
+
+Nose determines which functions are tests by searching for functions
+beginning with "test" in their name.
+
@pelson

pelson Nov 19, 2012

Member

A note in here would be useful regarding @cleanup.

@pelson pelson commented on an outdated diff Nov 19, 2012

doc/devel/testing.rst
+ def test_simple_fail():
+ '''very simple example test that should fail'''
+ assert_equal(1+1,3)
+
+Note that the first argument to the
+:func:`~matplotlib.testing.decorators.knownfailureif` decorator is a
+fail condition, which can be a value such as True, False, or
+'indeterminate', or may be a dynamically evaluated expression.
+
+Creating a new module in matplotlib.tests
+-----------------------------------------
+
+Let's say you've added a new module named
+``matplotlib.tests.test_whizbang_features``. To add this module to
+the list of default tests, append its name to ``default_test_modules``
+in :file:`lib/matplotlib/__init__.py`.
@pelson

pelson Nov 19, 2012

Member

Might be worth mentioning here our desire to mostly have relevant test module names (e.g. test_collections.py would test features written in the collections.py module).

@pelson pelson commented on an outdated diff Nov 19, 2012

doc/devel/testing.rst
+
+`Travis CI <http://travis-ci.org/>`_ is a hosted CI system "in the cloud".
+
+Travis is configured to receive notifications of new commits to GitHub repos
+(via GitHub "service hooks") and to run builds or tests when it sees these new
+commits. It looks for a YAML file called ``.travis.yml`` in the root of the
+repository to see how to test the project.
+
+Travis CI is already enabled for the `main matplotlib GitHub repository
+<https://github.com/matplotlib/matplotlib/>`_ -- for example, see `its Travis
+page <http://travis-ci.org/#!/matplotlib/matplotlib>`_.
+
+If you want to enable Travis CI for your personal matplotlib GitHub repo,
+simply enable the repo to use Travis CI in either the Travis CI UI or the
+GitHub UI (Admin | Service Hooks). For details, see `the Travis CI Getting
+Started page <http://about.travis-ci.org/docs/user/getting-started/>`_.
@pelson

pelson Nov 19, 2012

Member

May be worth mentioning that this isn't strictly necessary and that doing so means that travis-ci has to do more work (sometimes unnecessarily)...

Member

pelson commented Nov 19, 2012

Bravo! 👍 with the few suggestions I made.

Thanks @mdboom .

Owner

mdboom commented Nov 19, 2012

I (believe) I have addressed all of the comments above.

Member

pelson commented Nov 20, 2012

I (believe) I have addressed all of the comments above.

All looks good to me. Thanks @mdboom.

@pelson pelson added a commit that referenced this pull request Nov 20, 2012

@pelson pelson Merge pull request #1512 from mdboom/coding_guidelines
Reorganization of the developer docs to make it easier to get up to speed with the matplotlib development process.
1fa3318

@pelson pelson merged commit 1fa3318 into matplotlib:v1.2.x Nov 20, 2012

1 check passed

default The Travis build passed
Details

mdboom deleted the mdboom:coding_guidelines branch Aug 7, 2014

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