Code

Opened 13 months ago

Closed 12 months ago

Last modified 11 months ago

#20104 closed Bug (fixed)

Make versionchanged directive less prone to mis-use.

Reported by: carljm 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 13 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 13 months ago by aaugustin

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

comment:2 Changed 13 months ago by jcatalan

  • Owner changed from nobody to jcatalan
  • Status changed from new to assigned

comment:3 Changed 13 months 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 13 months 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 djangodocs.py?

comment:5 Changed 13 months ago by aaugustin

I don't think a test is required here.

comment:6 Changed 13 months ago by aaugustin

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 13 months ago by timo

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 Changed 13 months 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 13 months ago by 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!

comment:10 Changed 13 months ago by carljm

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 13 months ago by carljm

  • 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 13 months 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 12 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 12 months ago by Claude Paroz <claude@…>

In 78c842a3230f026ad678d243e5459cd6b314d99a:

Adapted uses of versionchanged/versionadded to the new form.

Refs #20104.

comment:15 Changed 11 months ago by Ramiro Morales <cramm0@…>

In a6ff705aea7ffcfb5477ce8c94a6cc1cd70f4bb1:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.