Opened 16 years ago

Closed 15 years ago

Last modified 13 years ago

#7977 closed (fixed)

add geo support to admindocs

Reported by: andrepleblanc@… Owned by: Karen Tracey
Component: GIS Version: dev
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: no UI/UX: no

Description

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

Attachments (5)

r8093.diff (1.3 KB ) - added by andrepleblanc@… 16 years ago.
admindocs support
admindocs_field_types.diff (15.4 KB ) - added by Cliff Dyer 16 years ago.
Patch that allows fields to define type in docstrings
admindocs_field_types_classattr.diff (15.9 KB ) - added by Cliff Dyer 15 years ago.
Patch that allows fields to define type in class attribute
admindocs_field_types_revised.diff (17.2 KB ) - added by Cliff Dyer 15 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 15 years ago.
Moves tests and related files into tests subdirectory

Download all attachments as: .zip

Change History (29)

by andrepleblanc@…, 16 years ago

Attachment: r8093.diff added

admindocs support

comment:1 by jbronn, 16 years ago

Component: UncategorizedGIS
Owner: changed from nobody to jbronn
Version: SVNgis

comment:2 by jbronn, 16 years ago

Status: newassigned

comment:3 by Eric Holscher, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by Brett Hoerner, 16 years ago

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 by Brett Hoerner, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by Peter Baumgartner, 16 years ago

Cc: pete+djangotrac@… added

comment:8 by anonymous, 16 years ago

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").

in reply to:  8 comment:9 by Cliff Dyer, 16 years ago

Replying to anonymous:

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

by Cliff Dyer, 16 years ago

Attachment: admindocs_field_types.diff added

Patch that allows fields to define type in docstrings

comment:10 by Cliff Dyer, 16 years ago

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 by Alex Gaynor, 16 years ago

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 by Cliff Dyer, 16 years ago

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

comment:13 by Cliff Dyer, 16 years ago

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 by Cliff Dyer, 16 years ago

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 by Cliff Dyer, 15 years ago

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.

by Cliff Dyer, 15 years ago

Patch that allows fields to define type in class attribute

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

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 by Eric Holscher, 15 years ago

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 by Cliff Dyer, 15 years ago

milestone: 1.2

by Cliff Dyer, 15 years ago

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

comment:19 by Cliff Dyer, 15 years ago

Owner: changed from jbronn to Karen Tracey
Status: assignednew

by Cliff Dyer, 15 years ago

Moves tests and related files into tests subdirectory

comment:20 by Karen Tracey, 15 years ago

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 by Karen Tracey, 15 years ago

(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 by Karen Tracey, 15 years ago

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

comment:23 by Karen Tracey, 15 years ago

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

r11841 from trunk.

comment:24 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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