Add a monkey-patch to VariableCommentPicker to make auto_member_order work.

[Sraw]

I have tested this PR on your autodoc_examples.py.

But I didn't add some tests about it. Not sure is it needed or not.

[njsmith]

Thanks for taking this on!

So first... apparently the tests are failing because the module we're trying to import from doesn't exist on sphinx 1.5. That's sphinx for you I guess. Gotta do something about that before we can merge this. I see two options:

• Put a try/except around the new code.
• Make sphinx 1.6 the minimum version we support. This would involve: (a) adding a >= 1.6 to the install_requires line in setup.py, (b) adding a release note documenting the new minimum version, (c) modifying .travis.yml to stop testing < 1.6. Probably we should make the test versions SPHINX_VERSION="~= 1.6" and SPHINX_VERSION=">= 1.7" to keep some multi-version test coverage.

The latter is a little more work, but if you feel comfortable doing it then it's probably what we want to do anyway (and might simplify testing).

Speaking of release notes, you should also add one for this change :-). I guess the "auto_member_def order works now" release note would be newsfragments/13.bugfix.rst (referring to the original issue being fixed), and the "drop support for sphinx < 16" would be newsfragments/14.removal.rst (referring to this PR, since that's where the discussion is about that is). Check out the newsfragments/ directory for some more documentation and examples.

Okay, and the final question is about testing this. Ugh. Testing sphinx is always kind of annoying, but... also kind of necessary, because sphinx changes things all the time, and this is particularly fragile because it's monkeypatching. So how can we test this.

I guess the current test_end_to_end is close, but not quite flexible enough to work. It's based on building some documentation, and then making assertions about the generated HTML. The problem is that right now these assertions are just "this string appears" or "this string does not appear", and that's not a very convenient way to make an assertion about what order members appear in. If it did regex matching then it might be doable, but still kind of awkward...

I think the best bet here is to give up on being clever and write a hard-coded test. Like, we can set up a little sphinx source directory like test-member-order-by-source/, with an appropriate conf.py (that enables auto_member_order = "by-source"), a .py file that defines a class that has a mix of async and synchronous methods, and a .rst file that does .. autoclass:: OurTestClass. Then to actually run the test, we can do something like:

def test_member_order_by_source(tmpdir):
    shutil.copytree(str(Path(__file__).parent / "test-member-order-by-source"),
                    str(tmpdir / "test-member-order-by-source"))
    subprocess.run(
        ["sphinx-build", "-v", "-nW", "-nb", "html",
         str(tmpdir / "test-member-order-by-source"), str(tmpdir / "out")])

    html = (tmpdir / "out" / "whatever-we-call-it.html").read_text("utf-8")

    # Make some assertions about what order the different members show up in the html
    # Maybe something like:
    # Listed in the order they should appear
    members = ["foo", "bar", "another_method", "this_one_is_async", "this_one_is_sync"]
    first_mention_offsets = [html.find(member) for member in members]
    assert all(offset >= 0 for offset in first_mention_offsets)
    # check they show up in the same order as our list
    assert sorted(first_mention_offsets) == first_mention_offsets

Does that make sense? Sorry it's kind of long -- I don't think any piece is too much, despite how many words it takes to describe, but if you have trouble with anything or don't have the time then just let me know and we'll figure something out.

[Sraw]

Well, I've noticed the failed tests.

I think the second option is better. I will take more time to think about it as I'm not familiar with your project's structure. Maybe in two days.

About the test, at first I was thinking about adding a new ExampleClassForOrderTest to test. But it is a little difficult to assert order as you have said. I will try to accomplish that.

[njsmith]

Whoops, just noticed that I had a bad copy-paste error in the middle of my comment. I've just edited it to fix it. So if you were finding it super hard to read, that was my fault :-)

[codecov]

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #14   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         253    264   +11     
  Branches       40     42    +2     
=====================================
+ Hits          253    264   +11

Impacted Files	Coverage Δ
sphinxcontrib_trio/__init__.py	100% <100%> (ø)	⬆️
tests/test_sphinxcontrib_trio.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 bbac843...39f1e09. Read the comment docs.

[Sraw]

Well, almost there except for failed assertion...

[Sraw]

As you can see, almost done. How does cov calculate? Does it mean I need to add a test case for that "if not hasattr"?

[njsmith]

Nicely done!

Yeah, the coverage is complaining about that hasattr line... but testing that doesn't really make sense, since there does not exist any version of sphinx where the line will pass, it's just an attempt to future-proof. So I'd add a comment on that line saying # pragma: no cover, which tells the coverage tool not to worry about it.

Also, we still need a newsfragment.

Other than those two things, I think this looks good! That test is quite clever :-)

[Sraw]

Fine, leart about coverage tool.

And then, so let's say now I should add two newsfragments which are "13.bugfix.rst" and "14.removal.rst". But I cannot see 1-12.

Could you explain a little bit more about this?

[Sraw]

Ops, I understand. That number is PR's number and issue's number.

Give me one second.

[Sraw]

Well well, I believe everything is OK.

[njsmith]

Oh, I'm a dork. I was all like "wait, why is @Sraw using no cover, when I clearly said... oh, I said no cover"

I'm sorry, I should have said (and thought I said) # pragma: no branch

(The difference is that no cover disables coverage for that statement, and possibly the following statements, entirely, and no branch just says "hey don't worry if this if statement never fails".)

[njsmith]

Eh, I'm just going to fix that myself and merge it, because it's such a silly little fix (and I feel silly for telling you the wrong thing)

[Sraw]

It doesn't matter :) I've really learnt a lot.
Thank you.

[njsmith]

You're welcome, and thank you!

And, no pressure, but if you'd like to keep contributing then we'd love to have you, so I'm sending you a github invite now. You can read more about this in our contributing documentation.

[spectras]

Just a quick word to thank all people involved in fixing this issue.