Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#7977 closed (fixed)

add geo support to admindocs

Reported by: andrepleblanc@… Owned by: Karen Tracey
Component: GIS Version: master
Severity: Keywords:
Cc: pete+djangotrac@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This patch adds support for the GIS Data Types to the admindocs app.

Attachments (5)

r8093.diff (1.3 KB) - added by andrepleblanc@… 8 years ago.
admindocs support
admindocs_field_types.diff (15.4 KB) - added by Cliff Dyer 7 years ago.
Patch that allows fields to define type in docstrings
admindocs_field_types_classattr.diff (15.9 KB) - added by Cliff Dyer 7 years ago.
Patch that allows fields to define type in class attribute
admindocs_field_types_revised.diff (17.2 KB) - added by Cliff Dyer 7 years ago.
Added documentation, and revised to only use first line of long docstrings.
admindocs_field_types_revised2.diff (17.3 KB) - added by Cliff Dyer 7 years ago.
Moves tests and related files into tests subdirectory

Download all attachments as: .zip

Change History (29)

Changed 8 years ago by andrepleblanc@…

Attachment: r8093.diff added

admindocs support

comment:1 Changed 8 years ago by jbronn

Component: UncategorizedGIS
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to jbronn
Patch needs improvement: unset
Version: SVNgis

comment:2 Changed 8 years ago by jbronn

Status: newassigned

comment:3 Changed 8 years ago by Eric Holscher

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 8 years ago by Brett Hoerner

I know this ticket / patch is GeoDjango specific, but in general DATA_TYPE_MAPPING should be made extensible outside of Django itself, right?

If you add your own field type the admindoc breaks (ie: UUIDField will always be a KeyError here, too).

comment:5 Changed 8 years ago by Brett Hoerner

Er, well, I guess it is just a normal dict, and can be imported elsewhere and mutated. I just wonder how permanent that "API" will be.

comment:6 Changed 8 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

I think Brett's broader point is valid here. This is a bit of an ugly patch, since it's so highly specific to one contrib app. Let's put on our brightly coloured thinking caps and do something better (dispatching to a "standard" function in each installed app in turn if there's a KeyError, for example).

It was probably either me or Jacob who had Eric move this to "d-d-n" during the Lawrence sprint last year. Moving to "Accepted" since, yes, we should do this. But not this way, preferably.

comment:7 Changed 7 years ago by Peter Baumgartner

Cc: pete+djangotrac@… added

comment:8 Changed 7 years ago by anonymous

If there's no "standard function" defined in the app, I'd still like to see it not throw an exception.

At minimum, DATA_TYPE_MAPPING should be a defaultdict that returns "Unknown field type" or "Field of type %s" % (field.class.name) if the type is not accounted for.

Better, the fields themselves carry information that can be used before falling back on the list (or after checking the list, or in place of the list). If checking on the field itself is the last thing done, fields can use the value on their superclasses by default, and then everything could fall back to a value on Field (which could be "Field of type %s" % field.class.name or "Unknown field type").

comment:9 in reply to:  8 Changed 7 years ago by Cliff Dyer

Replying to anonymous:

Oops. I forgot to log in. Last comment was by me.

Changed 7 years ago by Cliff Dyer

Attachment: admindocs_field_types.diff added

Patch that allows fields to define type in docstrings

comment:10 Changed 7 years ago by Cliff Dyer

A couple comments on my patch:

  1. I'm not sure I like that I changed the docstring on OneToOneField to a comment. It was the only field/relationship that had a docstring at all. Alternatively, the patch could be modified so that get_readable_field_data_type() could just take the first line of the docstring, and strip off the rest. That might be unexpected behavior, though.
  1. admindocs didn't have any associated unit tests that I could find, so I created some, but I only wrote tests for the function I modified. Some tests are better than none, right?

comment:11 Changed 7 years ago by Alex Gaynor

I'm not sure I like using docstrings for this as the -OO flag omits all docstrings, which someone might want to use in production.

comment:12 Changed 7 years ago by Cliff Dyer

Using docstrings is consistent with how admindocs extracts documentation from apps, though, so if you're using -OO, admindocs is useless anyway.

comment:13 Changed 7 years ago by Cliff Dyer

In my mind, using -OO is an explicit declaration that you aren't interested in the documentation. So it doesn't seem unreasonable that it would affect the output of admindocs. Also, the problem is mitigated by having a sensible default to fall back on. Even in the absence of any docstrings whatsoever, my patch will produce decent output. It also seemed cleaner than creating a Field.doc attribute, which would (mostly) duplicate the functionality of a docstring.

That said, it should be easy enough to redo the patch using attributes instead of docstrings, if the consensus is that it would be better that way.

comment:14 Changed 7 years ago by Cliff Dyer

Triage Stage: AcceptedDesign decision needed

I'm setting this to "Design Decision Needed," in order to get some feedback on whether to use the field type's docstrings (which mimics the behavior of the rest of admindocs, but has issues when running django with python -OO—as does the rest of admindocs), or to add a doc class attribute to each field type.

If we decide to go with docstrings, the patch is good to go.

comment:15 Changed 7 years ago by Cliff Dyer

Patch needs improvement: unset
Version: gisSVN

Here's a patch using class attributes. I went with field_description as the name.

Both patches apply cleanly, and the tests pass for both of them.

Changed 7 years ago by Cliff Dyer

Patch that allows fields to define type in class attribute

comment:16 in reply to:  15 Changed 7 years ago by Cliff Dyer

Replying to jcd:

Here's a patch using class attributes. I went with field_description as the name.

Both patches apply cleanly, and the tests pass for both of them.

Correction: I went with description for the name

comment:17 Changed 7 years ago by Eric Holscher

Applied this against trunk, and it worked as advertised. Beforehand it was giving me a KeyError on the Point field, afterwards it listed the point field.

A bit of whitespace in the admindocs/views.py didn't apply correctly, but otherwise it was fine.

comment:18 Changed 7 years ago by Cliff Dyer

milestone: 1.2

Changed 7 years ago by Cliff Dyer

Added documentation, and revised to only use first line of long docstrings.

comment:19 Changed 7 years ago by Cliff Dyer

Owner: changed from jbronn to Karen Tracey
Status: assignednew

Changed 7 years ago by Cliff Dyer

Moves tests and related files into tests subdirectory

comment:20 Changed 7 years ago by Karen Tracey

Resolution: fixed
Status: newclosed

(In [11833]) Fixed #7977: Fixed admindocs to use docstrings instead of a static array to locate type information.
Thanks J. Clifford Dyer.

comment:21 Changed 7 years ago by Karen Tracey

(In [11834]) [1.1.X] Fixed #7977: Fixed admindocs to use docstrings instead of a static array to locate type information. Thanks J. Clifford Dyer.

r11833 from trunk.

comment:22 Changed 7 years ago by Karen Tracey

(In [11841]) Apply doc addition that somehow was left out of r11833. Refs #7977.

comment:23 Changed 7 years ago by Karen Tracey

(In [11842]) [1.1.X] Apply doc addition that somehow was left out of r11834. Refs #7977.

r11841 from trunk.

comment:24 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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