Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#20104 closed Bug (fixed)

Make versionchanged directive less prone to mis-use.

Reported by: Carl Meyer Owned by: jcatalan
Component: Documentation Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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 and

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.

Attachments (1)

remove_ambiguity_for_versionchanged_directive.diff (1.9 KB) - added by jcatalan 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Aymeric Augustin

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

comment:2 Changed 6 years ago by jcatalan

Owner: changed from nobody to jcatalan
Status: newassigned

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by 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

comment:5 Changed 6 years ago by Aymeric Augustin

I don't think a test is required here.

comment:6 Changed 6 years ago by 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...

comment:7 Changed 6 years ago by Tim Graham

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

comment:8 Changed 6 years ago by 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!

comment:9 Changed 6 years ago by jcatalan

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

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

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by Carl Meyer

Has patch: set
Needs documentation: set
Patch needs improvement: set

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

comment:12 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

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.

comment:14 Changed 6 years ago by Claude Paroz <claude@…>

In 78c842a3230f026ad678d243e5459cd6b314d99a:

Adapted uses of versionchanged/versionadded to the new form.

Refs #20104.

comment:15 Changed 6 years ago by Ramiro Morales <cramm0@…>

In a6ff705aea7ffcfb5477ce8c94a6cc1cd70f4bb1:

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

Note: See TracTickets for help on using tickets.
Back to Top