Code

Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#7977 closed (fixed)

add geo support to admindocs

Reported by: andrepleblanc@… Owned by: kmtracey
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@… 6 years ago.
admindocs support
admindocs_field_types.diff (15.4 KB) - added by jcd 5 years ago.
Patch that allows fields to define type in docstrings
admindocs_field_types_classattr.diff (15.9 KB) - added by jcd 5 years ago.
Patch that allows fields to define type in class attribute
admindocs_field_types_revised.diff (17.2 KB) - added by jcd 4 years ago.
Added documentation, and revised to only use first line of long docstrings.
admindocs_field_types_revised2.diff (17.3 KB) - added by jcd 4 years ago.
Moves tests and related files into tests subdirectory

Download all attachments as: .zip

Change History (29)

Changed 6 years ago by andrepleblanc@…

admindocs support

comment:1 Changed 6 years ago by jbronn

  • Component changed from Uncategorized to GIS
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to jbronn
  • Patch needs improvement unset
  • Version changed from SVN to gis

comment:2 Changed 6 years ago by jbronn

  • Status changed from new to assigned

comment:3 Changed 6 years ago by ericholscher

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 5 years ago by bretthoerner

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 5 years ago by bretthoerner

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 5 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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 5 years ago by baumer1122

  • Cc pete+djangotrac@… added

comment:8 follow-up: Changed 5 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 5 years ago by jcd

Replying to anonymous:

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

Changed 5 years ago by jcd

Patch that allows fields to define type in docstrings

comment:10 Changed 5 years ago by jcd

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 5 years ago by Alex

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 5 years ago by jcd

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 5 years ago by jcd

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 5 years ago by jcd

  • Triage Stage changed from Accepted to Design 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 follow-up: Changed 5 years ago by jcd

  • Patch needs improvement unset
  • Version changed from gis to SVN

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 5 years ago by jcd

Patch that allows fields to define type in class attribute

comment:16 in reply to: ↑ 15 Changed 5 years ago by jcd

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 4 years ago by ericholscher

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 4 years ago by jcd

  • milestone set to 1.2

Changed 4 years ago by jcd

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

comment:19 Changed 4 years ago by jcd

  • Owner changed from jbronn to kmtracey
  • Status changed from assigned to new

Changed 4 years ago by jcd

Moves tests and related files into tests subdirectory

comment:20 Changed 4 years ago by kmtracey

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

(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 4 years ago by kmtracey

(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 4 years ago by kmtracey

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

comment:23 Changed 4 years ago by kmtracey

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

r11841 from trunk.

comment:24 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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.