fix versionadded & version changed directives

[2bndy5]

closes #981
I don't know when, but there was a change to how the versionlabels dict was declared (& possibly changed usage also) in Sphinx. This resulted in a failure to build a pdf when using versionadded, versionchanged, and deprecated directives.

See #981 for problematic output and concluding solution.

This PR fixes the pdfbuilder.py to compensate for the changes in Sphinx.

[lornajane]

Thanks! It is so good to get this input from Sphinx users, as maintainers we mostly use rst2pdf directly. A couple of points before I'm ready to merge:

Could we get a test for this please so that we might spot if we regress it?

How does this change affect documents produced directly with rst2pdf, are these directives used in the rst2pdf output or just passed back to Sphinx? If so, please add a test showing that usage also.

[2bndy5]

I mostly deal with libraries that require actual hardware to perform tests, so I'm afraid I'm rather inexperienced with creating pytests.

How does this change affect documents produced directly with rst2pdf, are these directives used in the rst2pdf output or just passed back to Sphinx?

This is a good question. I had assumed that pdfbuilder.py was specific to using sphinx (because of the sphinx-like term "builder"). More reason to perform tests (or in my case learn to use them).

[2bndy5]

do you have a recommendation of how/where to get the test's reference comparison output pdf?

From the Contributing guidelines:

reference holds the "correct" version of the PDF for this test. When adding a test, put the desired PDF output into this file as well as supplying the tests in the input folder.

As a pytest noob, I find this a little confusing. If I just use the first "passing" output from running the test, then it feels counterproductive to do a comparison when the test doesn't ERROR out.

Also, I had no idea this lib was pinned to Sphinx v1.8.5. I'm afraid that might complicate the dependencies part of the proposed test. I found the commit that introduced the breaking change solved by this PR. That commit predates Sphinx v1.8.5 (specifically released in v1.7.2)

[nfraprado]

The idea is to create a simple document inside tests/input that relies on the change you did in this PR to work correctly. Then, after you run pytest, the output pdf for that document will be placed in tests/output. You should then copy that pdf to tests/reference, so that future pytest runs compare the generated pdf for your test with the reference pdf you just added.

So, the output pdf should differ from the reference one before your PR (making the test fail), but match it after the PR (making the test pass). Since, before your PR, this use case crashed rst2pdf it will generate an empty pdf causing the test to fail, which is fine, the procedure is the same :).

[nfraprado]

Regarding Sphinx, if the breaking change was prior to version 1.8.5, then we should be fine testing your PR with this version.

[2bndy5]

One last question before I have a go at making the test. Am I wrong in thinking that the versionadded, versionchanged, and deprecated directives are sphinx specific? Are these directives also defined by the rst specs?

I don't see them in the docutil's reStructuredText Markup Specification for directives. Nor do I see a link for base case specifications of rst to adhere in the Contributing Guidelines.

[nfraprado]

No, you're right, they're Sphinx-only: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html
And from what I can tell, pdfbuilder.py is indeed only used in Sphinx builds, so you should be fine with a single test using Sphinx.

Feel free to reach out if you have any issue with creating the test.

[2bndy5]

The number one reason I like to contribute to others' libraries is for learning opportunities; this PR is no exception 😊 .

With that said, the new "sphinx-versionmodified" test passed on all 3 targeted versions of python. I chose the "sphinx-versionmodified" title for the test so its not as ambiguous as "sphinx-issue981".

I noticed that in the Contributing guidelines, the instructions say to use a venv named env, but the gitignore file only ignores .venv. If I were a total python/github noob, then I would have likely commit a venv titled env according to instruction. 👎🏻

Also, I looked at the pre-commit-config.yml and followed the link to an issue in the comment about using a workaround. It would seem that issue was addressed in May 2020 and released in August 2020. This might serve as a separate issue, and I'm not sure if the workaround is needed anymore (I'm also new to pre-commit).

[akrabat]

Be aware that we use pytest as a test runner. The actual tests are more like integration tests where we create an PDF file and compare it to a "known good" one in reference.

[2bndy5]

compare it to a "known good" one in reference.

The pdf I added to "tests/reference" does contain the expected output (using pytest on my fork's master branch of rst2pdf which should be based on upstream/master), but I understand there's more to a pdf than just the displayed contents. Does this mean I did something wrong?

[akrabat]

The pdf I added to "tests/reference" does contain the expected output (using pytest on my fork's master branch of rst2pdf which should be based on upstream/master), but I understand there's more to a pdf than just the displayed contents. Does this mean I did something wrong?

Nope. That's correct. The PDF in tests/reference is intended to be manually confirmed to be true by a human. When the tests are run, a new PDF is generated and the internal "objects" in it are compared with the reference PDF file.

[2bndy5]

FYI tests still pass, but CI failed to upload artifacts from workflow. Normally I'd just re-run the workflow(s), but I don't see that option in the menus.

[nfraprado]

Yeah, that happened to me recently as well. Whenever you push something the CI re-runs, so perhaps you could try force pushing again without changing anything? Though I'm not sure if a push with no change would really trigger the CI...

[lornajane]

This is in great shape, thanks so much for taking the time!

[lornajane]

I'm confused about the state of the CI here and it's blocking me merging this right now. I'll come back to it when I can!

[2bndy5]

Seems to me there is a difference between the current master branch's CI workflow and my fork's master branch's CI workflow. I think its related to the recent addition of building against python v3.9.

I could rebase my fork's master branch to see if that helps, but that will likely require another PR approval as all current reviews would be disregarded.

[nfraprado]

I think @lornajane meant that the PR wasn't mergeable 1 hour ago because the CI was waiting for approval first, before running the tests. It seems fine now, though.

[lornajane]

Mmmmmm, not sure what was going on there, I had to reduce some of the protection settings on the branch to be able to merge it. Those strange CI jobs that don't exist and therefore don't pass were blocking it.