X Tutup
Skip to content

Download the depsy.org badge when building the html documentation#8446

Merged
NelleV merged 2 commits intomatplotlib:masterfrom
jkseppan:download-depsy-badge
Apr 8, 2017
Merged

Download the depsy.org badge when building the html documentation#8446
NelleV merged 2 commits intomatplotlib:masterfrom
jkseppan:download-depsy-badge

Conversation

@jkseppan
Copy link
Copy Markdown
Member

@jkseppan jkseppan commented Apr 8, 2017

This should avoid the mixed-content warning at https://matplotlib.org where currently only the Depsy badge is loaded via http.

Depsy bug: ourresearch/depsy#77

This should avoid the mixed-content warning at https://matplotlib.org
where currently only the Depsy badge is loaded via http.

Depsy bug: ourresearch/depsy#77
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Apr 8, 2017

Can't we remove the depsy badge? I really don't like the changes to the make.py and the fact that we are downloading stuff everytime we build the documentation (got a university IP address blocked once because of building matplotlib's documentation too much. I'd rather avoid that experience :) )

urllib requests aren't context handlers in Python 2
@jkseppan
Copy link
Copy Markdown
Member Author

jkseppan commented Apr 8, 2017

The badges were added in #5825 which superseded #5811, not much discussion on the reasons. Perhaps the Depsy 100th percentile badge next to the donation badges sends a message that this is a widely used project that deserves the donation.

@jkseppan
Copy link
Copy Markdown
Member Author

jkseppan commented Apr 8, 2017

Regarding downloading things every time the documentation is built: currently the badge gets downloaded by every viewer of the web page (and cached in the browser for at most half a day) so this change would reduce the load on depsy.org, assuming the site gets viewed more often than it gets built. In any case, I wrote the function so it uses a default image if the file cannot be downloaded, so you can disable the network and the build will still work (unless some other code needs the network).

@NelleV
Copy link
Copy Markdown
Member

NelleV commented Apr 8, 2017

I'd personally remove the badge. Potential donors are very unlikely to know about depsy.

If we are going to keep the badge, I'm fine downloading it, thought it'd be nice to move this code out of the make.py if we are still migrating to a more modern makefile.

The reason I am worried about this patch is that it brings complexity and maintenance cost for something that I believe has little to no gain in that particular context.

(Both you and potential reviewers should feel free to ignore these remarks. The code is clean and obviously works.)

@tacaswell
Copy link
Copy Markdown
Member

I am 👍 on keeping the depsy badge, it is a measure if impact of the library (if there is a better one is a different issue).

@NelleV I hear your concerns about making my if we need complexity to build the docs then we would have that complexity this way or with a Makefile.

@NelleV
Copy link
Copy Markdown
Member

NelleV commented Apr 8, 2017

As I said, the solution would be to remove the badge. The badge brings little to no useful information. Matplotlib is a very well known library and people who don't know it won't know Depsy either.

@jkseppan
Copy link
Copy Markdown
Member Author

jkseppan commented Apr 8, 2017

My experience with Makefiles is that they tend to accrete a lot of complexity and become a maintenance burden. Are there more details somewhere of the proposed documentation build system replacement?

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Apr 8, 2017
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Apr 8, 2017

@jkseppan It's the "new" sphinx build system. It has been the default for at least the past 10 years.
There's been discussion on whether to migrate or not to it (I don't remember where).
In any case, this discussion is a bit futile, considering we can't migrate to the new build system right now (because of 15 years of hacking the make.py file).

@NelleV NelleV merged commit f273178 into matplotlib:master Apr 8, 2017
@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Apr 8, 2017

  1. I honestly don't think matplotlib will drop out of the 100% percentile in any foreseeable future, but it feels wrong to hardcode that value in our files.
  2. (regarding @jkseppan's comments) Sphinx switched some time ago to an even simpler Makefile system, which basically defers everything to python -msphinx -- so it's really not any burden:
# Minimal makefile for Sphinx documentation
#

# You can set these variables from the command line.
SPHINXOPTS    =
SPHINXBUILD   = sphinx-build
SPHINXPROJ    = foo
SOURCEDIR     = .
BUILDDIR      = _build

# Put it first so that "make" without argument is like "make help".
help:
        @$(SPHINXBUILD) -M help "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)

.PHONY: help Makefile

# Catch-all target: route all unknown targets to Sphinx using the new
# "make mode" option.  $(O) is meant as a shortcut for $(SPHINXOPTS).
%: Makefile
        @$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)

@jkseppan jkseppan deleted the download-depsy-badge branch April 9, 2017 05:32
tacaswell pushed a commit that referenced this pull request Apr 14, 2017
Download the depsy.org badge when building the html documentation
@tacaswell
Copy link
Copy Markdown
Member

backported to v2.0.0-doc as 9dd5c5b

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.

4 participants

X Tutup