Skip to content
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

autodoc_member_order ignored #13

Closed
spectras opened this issue Jun 11, 2018 · 15 comments

Comments

@spectras
Copy link

commented Jun 11, 2018

Hello,

I am trying to get sphinx to handle async methods in my project. I had good results with sphinxcontrib-trio so far, but I cannot achieve the member ordering I want.

My sphinx configuration has autodoc_member_order = 'bysource' defined.
The directive seems not to be applied to asynchronous functions. I end up with class members ordered this way:

  1. regular members in source order, then.
  2. asynchronous members in alphabetical order.

Expected result:
All members together, in source order.

Help most welcome.

@njsmith

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Huh, that definitely sounds like a bug. Off the top of my head I can't think why this would be happening, either, since async and sync methods mostly get handled by the same code paths...

Well, actually, one thing to check is how Sphinx is even finding out the "source order" in the first place. If it's, like, running a regex over the source, and that regex is looking for def at the beginning of a line, then it might be broken for async functions in general...

@spectras

This comment has been minimized.

Copy link
Author

commented Jun 16, 2018

Hello, thanks for the fast answer.
I played around with this for a while, and it seems you are right: autodoc apparently extracts async functions and regular functions in different groups. I do not know why exactly.
Thanks for your time!

@spectras spectras closed this Jun 16, 2018

@njsmith njsmith reopened this Jun 16, 2018

@njsmith

This comment has been minimized.

Copy link
Member

commented Jun 16, 2018

So this still sounds like something that needs fixing :-). Do you think it needs to be fixed in sphinx, then? Is there a bug open with them? Maybe it's something that sphinxcontrib-trio could work around in the mean time?

@spectras

This comment has been minimized.

Copy link
Author

commented Jun 16, 2018

Well I do not have much time to spend on this and did not want to waste yours.

The code that does the sorting is in sphinx autodoc extension, in this file.

It uses the analyzer that was defined here, and it basically just delegates to ModuleAnalyzer.for_module().

The parsing is done by the Parser class.
And with a breakpoint I was able to check that deforders, as copied from the picker here, does not include entries for async methods.

And at last, VariableCommentPicker does the actual job of reading the AST, and does not include a visitor for AsyncFunctionDef. Thus the missing entries.

All of this happens in sphinx, I do not know what hooks are available or if it's possible (or even desirable to monkey patch stuff). Probably an issue there…

… and yes, there is an awaiting PR here: sphinx-doc/sphinx#4847

@Sraw

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

I am also suffering this issue. So without waiting, we have nothing to do?

@njsmith

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@Sraw Not necessarily... judging from that PR, it might be as simple as adding a monkey-patch to sphinxcontrib-trio:

from sphinx.pycode.parser import VariableCommentPicker

if not hasattr(VariableCommentPicker, "visit_AsyncFunctionDef"):
    VariableCommentPicker.visit_AsyncFunctionDef = VariableCommentPicker.visit_FunctionDef

Looking at sphinx/pycode/parser.py, there's also a DefinitionFinder class that at first glance might look like it needs patching as well... but as far as I can tell, there's nothing in sphinx that ever actually uses that class, so it doesn't matter.

@Sraw

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Let's say, if I add the snippet you give to conf.py, will it work as expected?

Let me have a test...

@Sraw

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

No, it only works as dreamed... I think maybe I should find a way to mock this class globally.

@njsmith

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Huh, that's weird. That snippet does patch the class globally, and it looks like it should work...

@Sraw

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Ops, I delete _build and regenerate the whole docs. And it works.
Thank you.

@spectras

This comment has been minimized.

Copy link
Author

commented Jul 19, 2018

That's a great workaround indeed.
I always forget sphinx' conf is actually python code.

@njsmith

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

Anyone want to submit a PR to add this workaround to sphinxcontrib-trio itself, so people can get the benefit without tracking down this issue?

@Sraw

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

I will, in hours.

@njsmith

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Fixed by #14

@njsmith njsmith closed this Aug 6, 2018

@njsmith

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

I just realized that we never actually made a release after fixing this. So for anyone who's been waiting... sphinxcontrib-trio v1.0.2 is out now, with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.