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

FIX: No more workaround #387

Merged
merged 2 commits into from Jun 5, 2018

Conversation

Projects
None yet
4 participants
@larsoner
Contributor

larsoner commented Jun 5, 2018

I suspect that this will die on older versions of Sphinx, in which case I'll probably add a context manager to the sphinx_compatibility submodule.

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jun 5, 2018

This look good. I think this can be merged before the release today.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 5, 2018

Codecov Report

Merging #387 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   93.32%   93.33%   +<.01%     
==========================================
  Files          27       27              
  Lines        1949     1951       +2     
==========================================
+ Hits         1819     1821       +2     
  Misses        130      130
Impacted Files Coverage Δ
sphinx_gallery/tests/test_gen_gallery.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0085e08...a809518. Read the comment docs.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jun 5, 2018

I locally checked out the 1.5-release branch of sphinx and it worked, so it looks like this should indeed work.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jun 5, 2018

Nice, thanks a lot for this! I saw the response on the sphinx issue but I was not sure how easy it was to use it.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jun 5, 2018

+10/-8 easy :)

@lesteve lesteve merged commit 1481813 into sphinx-gallery:master Jun 5, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 93.32%)
Details
codecov/project 93.33% (+<.01%) compared to 0085e08
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lesteve

This comment has been minimized.

Contributor

lesteve commented Jun 5, 2018

CIs are green merging, thanks a lot!

@larsoner larsoner deleted the larsoner:namespace branch Jun 5, 2018

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jun 6, 2018

I should have tested this on sphinx master, it looks like the problem is still here: https://travis-ci.org/sphinx-gallery/sphinx-gallery/jobs/388594944 (sphinx master is only tested during the daily Travis cron job).

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jun 6, 2018

Ahh rats. Well I can make a PR to revert it.

Any objection to adding sphinx master as a standard Travis job? We don't seem to be anywhere near having problems with testing time.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jun 6, 2018

Any objection to adding sphinx master as a standard Travis job? We don't seem to be anywhere near having problems with testing time.

The reason I put it in a daily Travis cron job is that in 99% of the cases, it's not the fault of the PR if the build with sphinx master fails at all. Also it's not really urgent generally so we can leave it fail for a few days, whereas I find having all PRs red for a few days without a good reason is annoying.

To be honest: I just followed what we do in scikit-learn for numpy, scipy, cython master and that has worked quite well for us so far.

@larsoner larsoner referenced this pull request Jun 12, 2018

Merged

MRG: Fix for 1.8.0 dev #391

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