update Jinja2 #191
update Jinja2 #191
Conversation
Markup, a unpinned dependency of Jinja2, decided to remove a deprecated function that Jinja relied, causing Jinja2 to throw an error sometime in Feb 2020. Jinja2 has upgraded and recommends affected users upgrade rather than pinning an old version of Markup. This PR incorporates that update into sphinx-js
|
This is an improvement as is, but is insufficient to fix the current build issues. Sphinx also needs an update to CircleCI is running tests on the branch because it's off the main repo, but isn't reporting them on PRs. No idea why. I'll see if that's quick to fix at the same time |
|
8 of the 9 new errors introduced by Sphinx There is one case of |
sphinx relies on other deprecated Jinja functions that are now removed, such that in order to get a working build we have to update Sphinx to 4.x at the same time. Note that this introduced behavioral changes that break our tests: `"type()"` is returned instead of `*type*` and `autofunction_static` no longer works. This at least installs deps and gets to the testrunner before it falls over, which is an improvement
|
Pushing a version with the Sphinx update before I put this down for a bit. |
this enables running single tests using something like `tox -e pyXX -- tests/file.py::TestClass::test_method`
earlier versions of parsimonious relied on a deprecated api call that is removed in 3.10, and this update removes a warning at test time
this matches the style of other tests around it and improves the readability of the expected result
this matches the style of other tests around it and improves the readability of the expected result
…claration previously we expected jsdoc to return the line number of the constructor when documenting a class, which was considered "not ideal".
|
The behavior around constructors has changed in the updated libraries. They are still artificially marked In |
this changes the class and class constructor tests to match what should be expected from the test js file. JSDoc behavior changed and the brittle workarounds of prior old behavior need to be re-examined. This doesn't fix the build, but it does fix some failures while introducing others
sphinx-js assumes semver, but both Jinja2 and Sphinx have removed functionality in minor version bumps that broke this library. This pins specific versions instead of relying on major version ranges.
Sphinx 4.3 removed the simple display_prefix string and replaced it with a function that is expected to return a list of nodes, which can be created from a string. This implements that for static, and changes both the sample code and the test to expect a more descriptively named static function intended to make debugging and code tracing easier
|
Types are incorrectly being printed as This is difficult to trace with the VSCode python debugger because it has no apparent programmatic watch or ability to search through variables in the namespace and these documents get deep. I followed the fields renderer inward using the On the other end |
This is the lowest version number of sphinx where any tests succeed. This will minimize the number of external changes to examine while trying to restore the test suite
|
I thought to bissect versions of Sphinx to determine where the breaking change was introduced, but it's difficult to even recreate a historically working version. The last known working version of Sphinx was 3.5.2, but it's not simple to wind back version numbers and get a working test run. This latest commit pins Sphinx at 4.1.0, which is the first version that uses jinja2 3.0 and the first version where any of the tests pass. This should at least help narrow down the changes, and we can ignore things introduced in 4.2+.x (for now) I've also confirmed that built-in sphinx |
|
Ok, with the main branch working it was easier to isolate the breaking changes. Sphinx 4.0.x requires downgrading MarkupSafe to <2.0 so I did that, built this with MarkupSafe 1.1.1 and {Sphinx 3.x.x, 4.0.x}. When I try to build with Sphinx 4.1.0 the test failures described above show up. This is one example: See: https://github.com/mozilla/sphinx-js/tree/190-update-jinja-take2 up next: WTF changed in Sphinx 4.1.0 and how do we work around it |
|
The changelog was not useful so I bisected every change in Sphinx between 4.1.0 and 4.0.3 and narrowed it to: edit: I grabbed the wrong sha initially so I corrected that, added a complete record of my local tests |
|
The change is part of PR 9155, which is a fix for Sphinx issue 8945. |
|
I've been tracing VSCode's python debugger is fine but in order to investigate these failures the setting
When the is an instance of a My next place to dig in is the |
|
Still thinking about this but I'm unlikely to get much done here before July. Hopefully the breadcrumb notes I've left are more valuable than annoying if someone gets impatient and needs to run ahead of me. I approved and merged #195 assuming that being able to narrow down the change would help me get a fix in quickly but that has not been the case. sphinx-doc/sphinx@4534d2d has a weird pending_xref special case for python that I didn't think was relevant to our JS case. However, in the debugger I see one in the doctree. It's the node inside the parenthesis that wrap the argument types |
|
Some progress: I previously determined that our unsolved errors shows up with the following sphinx commit: sphinx-doc/sphinx@4534d2d I've locally created a new set of tests called from tests.testing import SphinxBuildTestCase
class Tests(SphinxBuildTestCase):
"""Tests which require our big JS Sphinx tree to be built.
Yes, it's too coupled.
Many of these are renderer tests, but some indirectly test JS analysis.
These latter are left over from when JS was the only supported language and
had its assumptions coded into the renderers.
"""
def test_autofunction_minimal(self):
"""Make sure we render correctly and pull the params out of the JS code
when only the function name is provided."""
self._file_contents_eq(
'autofunction_minimal',
'linkDensity(node)' + DESCRIPTION + FIELDS)
DESCRIPTION = """
Return the ratio of the inline text length of the links in an
element to the inline text length of the entire element."""
FIELDS = u"""
Arguments:
* **node** (*Node*) -- Something of a single type
Throws:
**PartyError|FartyError** -- Something with multiple types and a
line that wraps
Returns:
**Number** -- What a thing
"""and the associated /**
* Return the ratio of the inline text length of the links in an element to
* the inline text length of the entire element.
*
* @param {Node} node - Something of a single type
* @throws {PartyError|FartyError} Something with multiple types and a line
* that wraps
* @returns {Number} What a thing
*/
function linkDensity(node) {
const length = node.flavors.get('paragraphish').inlineLength;
const lengthWithoutLinks = inlineTextLength(node.element,
element => element.tagName !== 'A');
return (length - lengthWithoutLinks) / length;
}with a few files in a minimized Using the specific sphinx commit and python3.9 I traced one whole run of the failing test, including library functions. Unfortunately that wasn't so useful. Even with watched variables it was difficult to track the relevant bits of state. Sphinx uses regular expressions and state machines to dynamically assemble and expand trees. Even line by line I couldn't follow how we ended up in a bad state. I went back to looking at the specific changes in 4534d2d1a5755c8cbc9ef4327eab7e34a85a7de8 and set breakpoints around the changes in both files. The
Items 2 and 3 from that list correspond to the two differences in the actual string (compared to the expected string) resulting in our test failure. List item 1 appears to be the variable name which precedes the type in the docstring, and also appears to be correctly represented. If list items 2 and 3 had no rolename, instead of func, they would probably also come through as literalstrong and render appropriately. Next steps:
The sphinx change associated with our problem added some additional logic here that should only have applied for python classes. Its possible this was always a benign bit of extra state, and even though we're in the JS domain we're being caught by this python-specific code in a general utility class unexpectedly |
|
The previous version of sphinx indeed ignored the role and had no inliner: refnode = addnodes.pending_xref('', refdomain=domain, refexplicit=False, reftype=rolename, reftarget=target)
refnode += contnode or innernode(target, target)
if env:
env.get_domain(domain).process_field_xref(refnode)
return refnodeWhen I revert just the body of the In the new code an inliner is passed to The inliner is part of the new code. It's originally retrieved from the directive ('js:function') in It's not immediately obvious to me what to do with this from sphinx-js code |
|
Jinja2 has been updated This is mostly useful for context on some other issues, but it is linked directly so I'm not worried about discoverability. It can be closed for now. |
Markup, a unpinned dependency of Jinja2, decided to remove a deprecated function that Jinja relied on, causing Jinja2 to throw an error sometime in Feb 2020.
Jinja2 has upgraded and recommends affected users upgrade rather than pinning an old version of Markup.
This PR incorporates that update into sphinx-js