Opened 11 years ago

Closed 11 years ago

Last modified 11 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

Description

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.

Attachments (1)

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

Download all attachments as: .zip

Change History (16)

comment:1 by Aymeric Augustin, 11 years ago

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

comment:2 by jcatalan, 11 years ago

Owner: changed from nobody to jcatalan
Status: newassigned

comment:3 by jcatalan, 11 years ago

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 by jcatalan, 11 years ago

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?

comment:5 by Aymeric Augustin, 11 years ago

I don't think a test is required here.

comment:6 by Aymeric Augustin, 11 years ago

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 by Tim Graham, 11 years ago

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.

comment:8 by jcatalan, 11 years ago

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 by jcatalan, 11 years ago

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!

comment:10 by Carl Meyer, 11 years ago

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 by Carl Meyer, 11 years ago

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 by anonymous, 11 years ago

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 by Claude Paroz <claude@…>, 11 years ago

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 by Claude Paroz <claude@…>, 11 years ago

In 78c842a3230f026ad678d243e5459cd6b314d99a:

Adapted uses of versionchanged/versionadded to the new form.

Refs #20104.

comment:15 by Ramiro Morales <cramm0@…>, 11 years ago

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