Skip to content

Something wrong with our sphinx compatibility matrix after #14... #15

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

Closed
njsmith opened this issue Aug 6, 2018 · 4 comments
Closed

Comments

@njsmith
Copy link
Member

njsmith commented Aug 6, 2018

@Fuyukai reports running into a strange crash: https://gitter.im/python-trio/general?at=5b6730c63a5a2d2f99f91e22

This seems to be an incompatibility between #14 and whatever version of sphinx she's using (which it sounds like is some kind of 1.6.x, though I haven't confirmed this).

When we merged #14, we thought that it worked with any sphinx 1.6.x or 1.7.x. First problem is, we didn't actually add a >= 1.6 to this package's install_requires in setup.py, whoops.

Second problem: I'm not sure that >= 1.6 is the right thing! @Fuyukai's problem seems to be with a 1.6.x release, and I just checked the latest 1.6.x release tag, and it doesn't have a sphinx.pycode.parser module: https://github.com/sphinx-doc/sphinx/tree/v1.6.7/sphinx/pycode

But somehow our CI tests are passing on 1.6, but weren't on 1.5? Something weird is going on here.

@njsmith
Copy link
Member Author

njsmith commented Aug 6, 2018

Oh, should probably CC @Sraw

@njsmith
Copy link
Member Author

njsmith commented Aug 6, 2018

....oh, our tests use sphinx ~= 1.6, but what they should be using is sphinx ~= 1.6.0. (The ~= operator assumes that in the version number you give it, the last segment is the micro version that you're OK with having change.) So actually we aren't testing against 1.6.x at all, and since #14 was merged our actual minimum version has been 1.7. Whoops.

@Sraw
Copy link

Sraw commented Aug 6, 2018

Well, yep, you are right. After reviewing commit, I think this problem exists also before merging #14 as I just change <1.6 to ~1.6. So sphinx==1.6.x is never tested lol.

I'm now trying to find a compatible way in sphinx==1.6.x.

njsmith added a commit to njsmith/sphinxcontrib-trio that referenced this issue Aug 6, 2018
@Sraw
Copy link

Sraw commented Aug 6, 2018

I found a possible way. In sphinx==1.6.7, they use a custom Grammar-py3.txt instead of built-in ast module. But in this txt, they actually define async_funcdef and funcdef. I guess we can use the similar way in this case.

try:
    from sphinx.pycode.parser import VariableCommentPicker
except ImportError:
    # Sphinx <1.7
    from sphinx.pycode import AttrDocVisitor
    if not hasattr(AttrDocVisitor, "visit_async_funcdef"):  # pragma: no branch
        AttrDocVisitor.visit_async_funcdef= AttrDocVisitor.visit_funcdef
else:
    if not hasattr(VariableCommentPicker, "visit_AsyncFunctionDef"):  # pragma: no branch
        VariableCommentPicker.visit_AsyncFunctionDef = VariableCommentPicker.visit_FunctionDef

I haven't tested, I am going on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants