apidoc: Restore support for legacy '_t'-suffix template files #12929
Conversation
|
@jayaddison I think the new approach is nice, thank you. I've pushed a commit to simplify the test by avoiding the In general I think the test suite is geared too much towards (slow) integration tests rather than simpler unit tests -- the proliferation of the test roots etc. A |
AA-Turner
changed the title
| @@ -53,6 +55,28 @@ def test_simple(make_app, apidoc): | |||
| print(app._warning.getvalue()) | |||
|
|
|||
|
|
|||
| def test_custom_templates(rootdir): | |||
There was a problem hiding this comment.
Given the new simplified test this could go into test_util/test_util_template.py instead...
There was a problem hiding this comment.
I think it's useful to have coverage here as confirmation that the reported regression has been solved; perhaps some unit testing on the template code does make sense too though.
And yep, almost every time I go to modify/add a testroot I question whether it is a good idea or not. In a way I think it's nice to have a compendium of examples of "this is how we think a given project should be displayed" - engineering-wise though, that seems unwieldly (currently?) and so I'll try to use unit testing more often.
There was a problem hiding this comment.
To provide more-actionable feedback, though: yep, if we keep this test as-is, I do think it should probably be relocated to test_util_template.py (or even a test_jinja2glue.py?) since it's exercising code that is only coincidentally apidoc-related.
There was a problem hiding this comment.
Oh, missed this sorry. Feel free to update as you see fit & ping me when done!
A
There was a problem hiding this comment.
Ok, I've started looking at restructuring the test.
- ✅ I like that the templates are simplified - no need to keep the irrelevant template content.
- ✅ Adding the assertion on the directory listing is sensible; it confirms that the inputs are as expected, and that a successful result isn't due to some other unrelated content.
- ❌ I'm not so keen on the updated
testroot- it's not really a complete Sphinx project root, and I don't think that's in the spirit of the testroots. - After more consideration I do think that the test -- because it is a regression fix -- should stay in the
apidocpart of the test suite.
I'll try to find a middle-ground that accommodates these.
There was a problem hiding this comment.
I'll try to find a middle-ground that accommodates these.
I'm not sure the changes I've pushed are ideal; some of the test framework is still a bit quirky. But I think it is cleaner than it was originally.
Almost - I'd like to do a quick sense-check to confirm whether the |
I think this is OK. The |
|
@AA-Turner I'm reasonably comfortable with these changes for merge now, but please feel free to re-question and/or adjust the test case structure; I don't mind taking some more time to try to improve it. Similarly, perhaps I'm being lazy by not adding a smaller unit-test over the |
|
I think fine as is; more tests are always nice but more important to fix the issue in hand. A |
|
Thanks @AA-Turner! |
Feature or Bugfix
Purpose
_tsuffix) template files are not discovered duringapidocrendering.Detail
.jinjatemplate suffix available within the Sphinx package itself -- but the operator can provide a--templatedirargument to prepend a named directory to the template search path..jinjaand then_tsuffixed files in the template search path, and return the first successful result.Relates