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

DOC: Add development guide .rst to zipline.io #1820

Merged
merged 1 commit into from Jun 8, 2017

Conversation

Projects
None yet
5 participants
@freddiev4
Contributor

freddiev4 commented May 26, 2017

Builds on #1807, fixes #1700, and fixes #495

This change is a step to remove information from the wiki as I believe zipline.io is a better place to add something like dev guidelines (along with other things)

One thing I'd like to update/add is a Contribution Requests section for larger contributions; and then start using the Beginner Friendly label more for smaller issues or issues that are clearly mapped out for how to fix them (which would require pruning of existing issues)

We could add a lot more content to this.

  • What the page is about and who it is for

Probably worth a separate PR:

  • Add a Contributions Request Section for larger projects
  • Add an FAQ/Frequent Dev Issues and how to fix them

@freddiev4 freddiev4 changed the title from DOC: Add development guide .rst to DOC: Add development guide .rst to zipline.io May 26, 2017

@coveralls

This comment has been minimized.

coveralls commented May 26, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling e86f1a1 on development-docs into 77235a6 on master.

@freddiev4 freddiev4 requested a review from richafrank May 26, 2017

@ssanderson ssanderson self-requested a review May 26, 2017

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented May 26, 2017

Also just remembered I'd like to add a section on how to format docstrings

@ssanderson

This comment has been minimized.

Member

ssanderson commented May 26, 2017

@richafrank

Nice!

Don't need to block on it, but would be good to discuss usage of the dev dockerfile as well.

.. code-block:: bash
# on linux
$ sudo apt-get install libopenblas-dev liblapack-dev gfortran

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Would it make sense for these to be links to the install docs, so we don't have to keep them both updated?

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

Agreed on this.

Also, FWIW, apt-get is specific to linux distributions like Ubuntu that use Debian packages (c.f. https://wiki.debian.org/Apt, https://wiki.debian.org/Packaging/BinaryPackage). Other linux distributions use different package managers. For example, Fedora uses dnf, and Arch uses pacman.

git checkout -b some-short-descriptive-name
The following section assumes you already have virtualenvwrapper and pip installed on your system. If you don't already have them, you'll need some C library dependencies. On Linux you can run:

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

I think like venv and pip are unrelated to these system dependencies?

$ pip install -r ./etc/requirements_dev.txt
$ pip install -r ./etc/requirements_blaze.txt
$ pip install -r ./etc/requirements_talib.txt
$ pip install coverage coveralls

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Why should devs install coveralls?

.. code-block:: bash
python setup.py built_ext --inplace

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

typo in build_ext

We use `flake8` for checking style requirements and `nosetests` to run zipline tests.
Before submitting patches or pull requests, please ensure that your changes pass when running:

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

You might mention that our CI will also run these checks.

$ nosetests --with-coverage
If you get an error running nosetests after setting up a fresh virtualenv, please try running deactivate zipline; workon zipline, where zipline is the name of your virtualenv.

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Should the bash commands be stylized in some way?

$ pip install -r ./etc/requirements_talib.txt
$ pip install coverage coveralls
Finally, you can install zipline in development mode by running:

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Does this actually install zipline, or just build any C extensions?

Commit messages
---------------
Standard acronyms to start the commit message with are:

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Feels a bit stilted to have the preposition before the verb at the end there. How about "Standard prefixes to start a commit message"?

later with handling the case (or raising appropriate errors) when
the algorithm has little cash on hand.
Pulling in Pull Requests (PRs)

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Do we actually want to use this method?

.. code-block:: bash
git clone git@github.com:quantopian/zipline.git

This comment has been minimized.

@richafrank

richafrank May 31, 2017

Member

Is this for quantopian devs or community devs? Those without commit rights to the quantopian repo will need to clone their own forks.

$ sudo apt-get install libopenblas-dev liblapack-dev gfortran
$ wget http://prdownloads.sourceforge.net/ta-lib/ta-lib-0.4.0-src.tar.gz

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

IMO, we should just make TA-Lib an optional dependency. Nothing in zipline actually needs it; it's just required to run the tests.

.. code-block:: bash
$ flake8 zipline

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

We actually run flake8 zipline tests in CI (which runs flake8 over the tests as well as the zipline package we distribute).

e.g.
.. code-block:: bash

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

bash seems like not the right code block header for this?

@@ -0,0 +1,141 @@
Zipline Development Guidelines

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

It feels like we probably want an opening paragraph explaining what this page is about and who it's for. Specifically, we should emphasize that the resources here are for users who want to contribute to Zipline or who want to install from source to make local changes to their copy of Zipline.

This comment has been minimized.

@ssanderson

ssanderson May 31, 2017

Member

Another thing that might be valuable here is a link to similar documents on larger, more established projects.

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 853964a on development-docs into 77235a6 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 3ba0cc2 on development-docs into 77235a6 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 3ba0cc2 on development-docs into 77235a6 on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 3ba0cc2 on development-docs into 77235a6 on master.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Jun 1, 2017

Added some more information but still would like to improve/add some stuff (edited the OP)

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 4593c48 on development-docs into 77235a6 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling c71faeb on development-docs into 77235a6 on master.

@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling c71faeb on development-docs into 77235a6 on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling c71faeb on development-docs into 77235a6 on master.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Jun 2, 2017

@ssanderson @richafrank pushed some more changes 🙂 whenever you guys have some time you could give it another look

@richafrank

Thanks! Feedback inline.

git checkout -b some-short-descriptive-name
The following section assumes you already have virtualenvwrapper and pip installed on your system. If you don't already have them, you'll need some C library dependencies. You can follow the `install guide`__ to get the appropriate dependencies.

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

I read this as saying that virtualenvwrapper and pip are required for the C libraries. What do you think? Should we swap the order of these sentences?

below.
Want to contribute? See our `development guidelines`__
__ http://zipline.io/development-guidelines.html

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Noticing that this link will be broken until we ship the docs.

.. code-block:: bash
# where zipline is the name of your virtualenv
$ deactivate Zipline

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Does capital versus not capital on the previous line matter?

.. code-block:: bash
$ flake8 Zipline tests

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Especially for case-sensitive environments, this should probably be lowercase "zipline" for the directory.

We do not currently have CI for OSX-64 bit builds, and do not support 32-bit builds.
__ https://travis-ci.org/

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Do you think it would be worth making these link to the zipline project at these sites?

Packaging
---------
To learn about how we build Zipline packages on `Anaconda`__ you can read `this`__ section in our release process notes.

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

The wording here doesn't strike me as quite right - we build conda packages on the various CI boxes and upload them to anaconda.org.

.. code-block:: bash
(master) $ git checkout -b PR-135
$ curl https://github.com/quantopian/Zipline/pull/135.patch | git am

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Is this how we want people to pull in PRs now?

Formatting Docstrings
---------------------
When adding or editing docstrings for classes, functions, etc, we use the numpy `HOWTO_DOCUMENT`__ file as the canonical reference.

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

What do you think about "we use numpy as the canonical reference" where "numpy" links to the HOWTO?

Contributing to the Docs
------------------------
If you'd like to contribute to the documentation, you can navigate to ``docs/source/`` where each `reStructuredText`__ or ``.rst``, file is a separate section here on zipline.io. To add a section, create a new file called ``some-descriptive-name.rst`` and add ``some-descriptive-name`` to ``appendix.rst``. To edit a section, simply open up one of the existing files, make your changes, and save them.

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

I have a little trouble parsing the first sentence here. Maybe:
"If you'd like to contribute to the documentation on zipline.io, you can navigate to docs/source/, where each reStructuredText__ file (.rst) is a separate section there."

@coveralls

This comment has been minimized.

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling e7eda0f on development-docs into 77235a6 on master.

@freddiev4 freddiev4 force-pushed the development-docs branch from e7eda0f to 2069d05 Jun 2, 2017

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Jun 2, 2017

Squashed commits and made some changes based off questions/comments.

@coveralls

This comment has been minimized.

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 2069d05 on development-docs into 9fe8076 on master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 2, 2017

Coverage Status

Coverage remained the same at 87.585% when pulling 2069d05 on development-docs into 9fe8076 on master.

@richafrank

Added some more. Let me know what you think 😄

.. code-block:: bash
$ nosetests

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Will this fail because we haven't gotten to the "Running Tests" section yet, and the user hasn't installed TA-lib?

Formatting Docstrings
---------------------
When adding or editing docstrings for classes, functions, etc, we use the `numpy`__ as the canonical reference.

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

"the numpy" seems awkward?

__ https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt
Pulling in Other Branches

This comment has been minimized.

@richafrank

richafrank Jun 2, 2017

Member

Feels like this section is somehow lacking, in that by the end, we haven't pulled in another branch (to master I assume).

This comment has been minimized.

@freddiev4

freddiev4 Jun 4, 2017

Contributor

I was thinking that contributors wouldn't need to merge anything into their local master branch and would just want to pull in other branches of zipline to build on someone else's branch or something.

This comment has been minimized.

@richafrank

richafrank Jun 5, 2017

Member

Ok, I think this section needs some more explanation if we want it to be immediately useful. As a newcomer, it's not clear to me what I'm try to accomplish by running these commands.

This comment has been minimized.

@richafrank

richafrank Jun 5, 2017

Member

We could instead leave it out for now, if you want to get the rest merged. Assuming there were no blockers from @ssanderson in the rest.

This comment has been minimized.

@freddiev4

freddiev4 Jun 5, 2017

Contributor

Sure. I'll leave it out for now.

@coveralls

This comment has been minimized.

coveralls commented Jun 4, 2017

Coverage Status

Coverage increased (+0.08%) to 87.666% when pulling 25e665f on development-docs into 9fe8076 on master.

@freddiev4 freddiev4 force-pushed the development-docs branch from 25e665f to c2af938 Jun 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 5, 2017

Coverage Status

Coverage remained the same at 87.666% when pulling c2af938 on development-docs into 3081386 on master.

---------
To learn about how we build Zipline packages, you can read `this`__ section in our release process notes.
__ https://anaconda.org/

This comment has been minimized.

@ssanderson

ssanderson Jun 5, 2017

Member

I don't think this is linked to anywhere?

Standard prefixes to start a commit message:
.. code-block:: bash

This comment has been minimized.

@ssanderson

ssanderson Jun 5, 2017

Member

Is bash the right section title for this?

@freddiev4 freddiev4 force-pushed the development-docs branch 4 times, most recently from f2bd4c0 to f0f83a8 Jun 6, 2017

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Jun 7, 2017

@richafrank @ssanderson I've made changes to address your comments and such; should be good for one final pass before merging.

Also thinking we should cut a release this week & am wondering what we should call it; do we follow http://semver.org/? or how do we decide on version numbers?

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2017

Coverage Status

Changes Unknown when pulling f0f83a8 on development-docs into ** on master**.

@richafrank

Biggest question was about TA-lib install order. Otherwise had some minor thoughts, if you want em.

.. code-block:: bash
git checkout -b some-short-descriptive-name

This comment has been minimized.

@richafrank

richafrank Jun 7, 2017

Member

Just noticing that we're inconsistent in using "$" to denote the bash prompt in our code block.

To finish, make sure `tests`__ pass.
__ http://zipline.io/development-guidelines.html#style-guide-running-tests

This comment has been minimized.

@richafrank

richafrank Jun 7, 2017

Member

Not a blocker, but I'm curious if we can refer to these pages with relative urls, instead of hardcoding the domain.

.. code-block:: bash
$ nosetests

This comment has been minimized.

@richafrank

richafrank Jun 7, 2017

Member

Running the tests here seems to require installing TA-lib, but earlier we were instructed to pip install -r ./etc/requirements_talib.txt and run tests as well. Did either of those require installing TA-lib?

This comment has been minimized.

@freddiev4

freddiev4 Jun 7, 2017

Contributor

Moved some of the wording around so the logical order makes more sense

.. note::
We do not currently have CI for OSX-64 bit builds, and do not support 32-bit builds.

This comment has been minimized.

@richafrank

richafrank Jun 7, 2017

Member

When we say here "support" for builds, does that mean that zipline won't run on 32-bit environments?

Packaging
---------
To learn about how we build Zipline packages, you can read `this`__ section in our release process notes.

This comment has been minimized.

@richafrank

richafrank Jun 7, 2017

Member

Let's be more explicit about what a package is, i.e. a conda package.

@freddiev4 freddiev4 force-pushed the development-docs branch from f0f83a8 to 3165320 Jun 7, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2017

Coverage Status

Coverage remained the same at 87.64% when pulling 3165320 on development-docs into 18aa25e on master.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Jun 7, 2017

image

@richafrank

This comment has been minimized.

Member

richafrank commented Jun 7, 2017

👍 from me

@freddiev4 freddiev4 merged commit 246eed2 into master Jun 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@freddiev4 freddiev4 deleted the development-docs branch Jun 8, 2017

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