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

fixing sphinx bug #242

Merged
merged 2 commits into from May 18, 2017

Conversation

Projects
None yet
6 participants
@choldgraf
Contributor

choldgraf commented May 16, 2017

Fixes #241 - looks like sphinx changed its API a little bit

@choldgraf choldgraf referenced this pull request May 16, 2017

Closed

sphinx 1.6.1 #241

@dopplershift

This comment has been minimized.

Contributor

dopplershift commented May 16, 2017

Well, in their defense it was _extensions...

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented May 18, 2017

pinging @Titan-C ...sorry to be obnoxious but this is a fairly annoying bug as it means that anybody who uses the latest sphinx won't be able to use sphinx gallery :-)

@@ -294,7 +294,8 @@ def setup(app):
app.add_stylesheet('gallery.css')
if 'sphinx.ext.autodoc' in app._extensions:
extensions_attr = '_extensions' if hasattr(app, '_extensions') else 'extensions'

This comment has been minimized.

@bsipocz

bsipocz May 18, 2017

Why not simply say:

 if 'sphinx.ext.autodoc' in getattr(app, '_extensions', 'extensions'):

This comment has been minimized.

@dopplershift

dopplershift May 18, 2017

Contributor

That returns the value of the '_extensions' attribute if it exists, which wouldn't be what's wanted here.

This comment has been minimized.

@bsipocz

bsipocz May 18, 2017

Looking at the line below, it seems like that is the thing wanted ultimately.

This comment has been minimized.

@dopplershift

dopplershift May 18, 2017

Contributor

It wants the value corresponding to either the '_extensions' attribute or the 'extensions' attribute, depending on which exists.

This comment has been minimized.

@bsipocz

bsipocz May 18, 2017

Hmm, well indeed the alternative is not spotless as 'extensions' doesn't mean anything as a string. Haven't tested that nesting works or not as I have to run now:

if 'sphinx.ext.autodoc' in getattr(app, '_extensions', getattr(app, 'extensions')):

This comment has been minimized.

@choldgraf

choldgraf May 18, 2017

Contributor

why not just getatr(app, '_extensions', app.extensions)? That's a little cleaner.

nvm apparently that won't work if app.extensions doesn't exist

This comment has been minimized.

@choldgraf

choldgraf May 18, 2017

Contributor

if 'sphinx.ext.autodoc' in getattr(app, '_extensions', getattr(app, 'extensions')):

I do think that this would work, though TBH I don't think it's as clear as just adding an extra line as is done in this PR.

This comment has been minimized.

@choldgraf

choldgraf May 18, 2017

Contributor

actually, thinking about it more, now I am 50/50 between the two options :-)

@Titan-C

This comment has been minimized.

Member

Titan-C commented May 18, 2017

@choldgraf thank you for having a look at this. Every time Sphinx has a release, they break something.
I do prefer your first implementation of first testing for the attribute compared to the nested getattr. I would add a comment above telling which version change in Sphinx this addresses. And also can you write in the CHANGES.rst an item linking to this PR.
Then I'll merge this tonight and make a release.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented May 18, 2017

@Titan-C how's that?

@Cadair

This comment has been minimized.

Contributor

Cadair commented May 18, 2017

Thanks for fixing this!

@amueller

This comment has been minimized.

Contributor

amueller commented May 18, 2017

looks good :)

@Titan-C Titan-C merged commit e05e7d9 into sphinx-gallery:master May 18, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Titan-C

This comment has been minimized.

Member

Titan-C commented May 18, 2017

Released Sphinx-Gallery is on PyPI now

@choldgraf choldgraf referenced this pull request May 18, 2017

Merged

Require sphinx < 1.6 #8634

@amueller

This comment has been minimized.

Contributor

amueller commented May 18, 2017

@Titan-C awesome, wanna update in sklearn?

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