Make versionchanged directive less prone to mis-use.

[Carl Meyer]

When the versionchanged directive is used like this:

.. versionchanged:: 1.6
   Here is some text.

This is a directive with two arguments, and it is rendered in the docs as "Changed in Django 1.5: Here is some text."

When it is used like this:

.. versionchanged:: 1.6

   Here is some text.

This is a directive with one argument, and content. It is rendered in the docs as "Changed in Django 1.5." The content is entirely ignored.

And in our docs, we often use it like this:

.. versionchanged:: 1.6

Here is some text.

This is a directive with one argument and no content, followed by a new paragraph which is unrelated to the directive (as far as Sphinx is concerned). This usage "works" (the following paragraph is rendered) but frequently leads to ambiguity about exactly how much of the following text is intended to be covered by the versionchanged declaration.

The reason for the content loss in the second case is that the versionchanged directive code checks if len(self.arguments) == 2 and if not, ignores the content too. Obviously, this behavior is a foot-gun for documentation authors. We have at least one case I've found in the docs (I would bet there are more) where the directive is used in the second form, and thus the information about _what_ changed is not rendered in the docs at all (see ​https://github.com/django/django/blob/aaec4f2bd8a63b3dceebad7804c5897e7874833d/docs/ref/contrib/gis/geos.txt#L279 and ​https://docs.djangoproject.com/en/dev/ref/contrib/gis/geos/#django.contrib.gis.geos.GEOSGeometry.hex).

I think the versionchanged directive should be changed in one of the following ways to eliminate this footgun:

1) Make it has_content = False so Sphinx will error if the second form is used, rather than silently hiding the content. (Change all instances of the second form to the first form.)

2) Make it accept content but not a second argument, so that Sphinx will error if the first form is used. (Change all instances of the first form to the second form.)

3) Make it accept both a second argument and content. Render the second argument on the same line as the text "Changed in version 1.6:" (this is how it is currently rendered), render the content as a boxed paragraph (like a note or warning).

I think that (3) is over-engineered, and (1) is atypical - most Sphinx directives that accept arbitrary text take this text as content rather than an argument (which allows Sphinx inline formatting to be applied in the content). So I favor (2).

In any case, I think that instances of the third form should be changed to one of the first two forms, to reduce ambiguity.

[Aymeric Augustin]

Same conclusions here, (2) is the way to go.

[jcatalan]

Hi, I've been looking at this. I agree with the (2) approach. I'm attaching a patch for the VersionChanged class code, including only one of the affected doc files changed (example of the 3rd case explained by carljm) to see what you think about it. If you think it's ok I can definitely make the changes on all the uses of versionchanged directive and send a pull request.

[jcatalan]

BTW, I tried to add a regression test on this one but I couldn't find where to put that. Do we have tests for djangodocs.py?

[Aymeric Augustin]

I don't think a test is required here.

[Aymeric Augustin]

The change looks good. It isn't possible to build the docs until all versionadded/changed directives have been fixed, so the next step is to include all of them in the patch...

[Tim Graham]

Instead of hardcoding "versionchanged" in the exception that's raised in djangodocs.py, I suggest using string interpolation and self.name so that the exception properly reflects either versionchanged or versionadded.

[jcatalan]

Yeap, you're right, I'll change that. And I'm also going to make the changes in all the occurrences of versionadded/changed directive. As soon as I have the new patch, I'll attach it. Thanks!

[jcatalan]

Ok, here's the PR with all the changes.

​https://github.com/django/django/pull/953

Hopefully, I'm not forgetting anything. Let me know what you think.Thanks!

[Carl Meyer]

Thanks for the pull request! I reviewed it and left some comments on github. I think the code is right, but there are some mistakes in the adjustment of existing uses of the directives. If you don't have time to make those adjustments, let me know and I can just make them myself when checking this in.

[Carl Meyer]

Also setting the "needs documentation" flag as a reminder to add documentation of how to use these directives for documentation authors.

[anonymous]

Great, thanks for the comments. I see I misunderstood many of the uses of the directive, sorry about that. I'll do the changes as soon as I can, don't worry.

[Claude Paroz <claude@…>]

In 4e25198ec298732409217321be10e1e06be2fcbd:

Fixed #20104 -- Changed VersionDirective in order to avoid ambiguity.

As explained in ticket #20104, the use of versionchanged/versionadded
was confusing.

To solve this ambiguity these directives no longer accept a second
argument but now they only receive the version number (1st arg) and then
a content with the proper comment.

[Claude Paroz <claude@…>]

In 78c842a3230f026ad678d243e5459cd6b314d99a:

Adapted uses of versionchanged/versionadded to the new form.

Refs #20104.

[Ramiro Morales <cramm0@…>]

In a6ff705aea7ffcfb5477ce8c94a6cc1cd70f4bb1:

[1.5.x] Tweaked a reST construct in template API docs. Refs #20104.