Skip to content

DOC: fill out dev docs - #9378

Merged
dstansby merged 6 commits into
matplotlib:masterfrom
mattip:devdocs
Oct 18, 2017
Merged

dstansby merged 6 commits into
matplotlib:masterfrom
mattip:devdocs

Conversation

@mattip

@mattip mattip commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

PR Summary

Trivial changeset to add more complete info for new devs getting started, fixes issue #9355

@dstansby dstansby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good overall, just a small change needed 👍

Comment thread doc/devel/testing.rst Outdated

We strongly recommend developing new features in a seperate virtualenv.

Install all the matplotlib dependencies_, and install matplotlib into the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Matplotlib" should always be uppercase in the text.

@dstansby dstansby added this to the 2.1.0-docs milestone Oct 12, 2017
Comment thread doc/devel/testing.rst Outdated
We strongly recommend developing new features in a seperate virtualenv.

Install all the matplotlib dependencies_, and install matplotlib into the
virtualenv with ``pip install -e matplotlib`` (but note the changes needed to

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, this isn't really my workflow. If you are developing, you need to pull down master from GitHub and pip install -e . from the GitHub directory. I also think that @anntzer just had a PR like this, or at least one that explicitly said how to pip install.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #9291 and use of python -mpip install -e .

Comment thread doc/devel/testing.rst Outdated
:file:`lib/matplotlib/tests`, and customizations to the pytest testing
infrastructure are in :mod:`matplotlib.testing`.

.. _dependencies: http://matplotlib.org/users/installing.html#dependencies

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a Sphinx cross-references and not http://matplotlib.org/...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

how do I do that to a particular anchor in the page, i.e. dependencies on the installing page?

@QuLogic QuLogic Oct 15, 2017 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You'd need to add a tag before the heading, but fortunately, there's already one there: install_requirements. You can use that name in the cross-reference instead of dependencies.

(This may be a bit difficult to find because doc/users/installing.html actually comes from INSTALL.rst in the top-level.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So this change is not needed any more?

@tacaswell

Copy link
Copy Markdown
Member

@mattip Thanks for doing this! Do you mind if I push some commits to your branch?

@mattip

mattip commented Oct 12, 2017

Copy link
Copy Markdown
Contributor Author

@tacaswell please go ahead

Comment thread doc/devel/testing.rst Outdated
Install all the Matplotlib dependencies_, and install matplotlib into the
virtualenv with ``pip install -e matplotlib`` (but note the changes needed to
``setup.cfg`` below) which will allow you to edit the code without
reinstalling.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still think this is repeating what's in devel/contributing.rst. I think we should avoid duplicating instructions. If you haven't already installed the development version of Matplotlib then testing doesn't make a lot of sense.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would just write "Install the latest version of Matplotlib as documented in http://matplotlib.org/devdocs/devel/contributing.html#retrieving-and-installing-the-latest-version-of-the-code. In particular, follow the instructions to use a local FreeType build." (fix the link, of course). And then remove the mention of localfreetype below.

(We also print a warning when tests are done on a nonlocal freetype version so I think that's enough.)

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks this looks helpful!

@mattip

mattip commented Oct 17, 2017

Copy link
Copy Markdown
Contributor Author

@dstansby can you please review/approve?

@dstansby

Copy link
Copy Markdown
Member

There's still a couple of places where 'Matplotlib' doesn't have an uppercase "M"; could you change them? Otherwise this looks good to go.

@mattip

mattip commented Oct 17, 2017

Copy link
Copy Markdown
Contributor Author

I pushed a changeset, but github is not responding?
@dstansby once you can see the changes, if I missed or overshot could you somehow mark them?

@mattip

mattip commented Oct 17, 2017

Copy link
Copy Markdown
Contributor Author

weird, I can see the changeset here on my fork but it is not triggering an update to the pull request

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still missing lots of "matplotlib" -> "Matplotlib". A couple of small edits

Comment thread doc/devel/contributing.rst Outdated
value.


Install matplotlib in developer mode

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Installing Matplotlib

Comment thread doc/devel/contributing.rst Outdated
Unfortunately, different versions of FreeType produce differently
formed characters, causing these image comparisons to fail. To make
them reproducible, matplotlib can be built with a special local copy
of FreeType. This is recommended for all matplotlib developers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Matplotlib etc....

Comment thread doc/devel/contributing.rst Outdated
meaning the result of a plot is compared against a known good result.
Unfortunately, different versions of FreeType produce differently
formed characters, causing these image comparisons to fail. To make
them reproducible, matplotlib can be built with a special local copy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Matplotlib

Comment thread doc/devel/contributing.rst Outdated
To make sure the tests run locally you must build against the correct version
of freetype. To configure the build system to fetch and build it either export
the env ``MPLLOCALFREETYPE`` as::
Building matplotlib for image comparison tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Matplotlib

@jklymak

jklymak commented Oct 17, 2017

Copy link
Copy Markdown
Member

Oh, I see, you are having issues uploading! I thought your ability to search/replace "matplotlib" should have been better ;-) Sorry about that. Still suggest changing "Install" to "Installing" The rest fo the headings are "-ing"...

@jklymak
jklymak dismissed their stale review October 17, 2017 15:35

Was based on uploading issues

@jklymak

jklymak commented Oct 17, 2017

Copy link
Copy Markdown
Member

You need to make sure your underlines extend the length of the titles....

@dstansby

Copy link
Copy Markdown
Member

Looks like a nice improvement, thanks a lot!

@dstansby
dstansby merged commit 401c1d7 into matplotlib:master Oct 18, 2017
lumberbot-app Bot pushed a commit that referenced this pull request Oct 18, 2017
dopplershift added a commit that referenced this pull request Oct 18, 2017
@mattip
mattip deleted the devdocs branch February 15, 2018 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants