#7977 closed (fixed)
add geo support to admindocs
Reported by: | 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)
Change History (29)
by , 16 years ago
Attachment: | r8093.diff added |
---|
comment:1 by , 16 years ago
Component: | Uncategorized → GIS |
---|---|
Owner: | changed from | to
Version: | SVN → gis |
comment:2 by , 16 years ago
Status: | new → assigned |
---|
comment:3 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:4 by , 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 , 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 , 16 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Design decision needed → 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 by , 16 years ago
Cc: | added |
---|
follow-up: 9 comment:8 by , 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").
comment:9 by , 16 years ago
Replying to anonymous:
Oops. I forgot to log in. Last comment was by me.
by , 16 years ago
Attachment: | admindocs_field_types.diff added |
---|
Patch that allows fields to define type in docstrings
comment:10 by , 16 years ago
A couple comments on my patch:
- 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.
- 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 , 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 , 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 , 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 , 16 years ago
Triage Stage: | Accepted → 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.
follow-up: 16 comment:15 by , 15 years ago
Patch needs improvement: | unset |
---|---|
Version: | gis → 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.
by , 15 years ago
Attachment: | admindocs_field_types_classattr.diff added |
---|
Patch that allows fields to define type in class attribute
comment:16 by , 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 , 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 , 15 years ago
milestone: | → 1.2 |
---|
by , 15 years ago
Attachment: | admindocs_field_types_revised.diff added |
---|
Added documentation, and revised to only use first line of long docstrings.
comment:19 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
by , 15 years ago
Attachment: | admindocs_field_types_revised2.diff added |
---|
Moves tests and related files into tests subdirectory
comment:20 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
admindocs support