Opened 16 years ago

Closed 13 years ago

Last modified 11 years ago

#7609 closed Bug (fixed)

Document that PositiveIntegerField and co have a misleading name

Reported by: Gergely Kontra <pihentagy+djangoproject@…> Owned by: Paul Collins
Component: Documentation Version: dev
Severity: Normal Keywords: name convention, rename
Cc: Paul Collins Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I find the PositiveIntegerField (and similar firends) name misleading. It allows zero.

Using UnsignedIntegerField or something like that cat solve that confusion.

Attachments (3)

UnsignedIntegerField.diff (623 bytes ) - added by jcrocholl 16 years ago.
Very simple patch to add UnsignedIntegerField and UnsignedSmallIntegerField as subclasses of PositiveIntegerField and PositiveSmallIntegerField.
doc_patch.diff (600 bytes ) - added by Paul Collins 13 years ago.
Update per comment 15
doc_patch_wo_link.diff (519 bytes ) - added by Paul Collins 13 years ago.
Same idea without the link to this ticket

Download all attachments as: .zip

Change History (22)

comment:1 by Adam Vandenberg, 16 years ago

Remembering my distant math major past, I suppose "NonNegativeInteger" would be more technically correct, but I'm rather OK with PositiveInteger including zero, it's what I expected.

comment:2 by Jeff Anderson, 16 years ago

Owner: changed from nobody to Jeff Anderson
Status: newassigned
Triage Stage: UnreviewedDesign decision needed

Calling it something else would be the "right" way to fix this. I'm +1 for calling it UnsignedInteger, because that is really what it is. This will preserve the current behaviour, and to make PositiveInteger act properly, we could simply add code to it to reject 0 when validating.

Writing a patch like this should be fairly trivial.

comment:3 by Julian Bez, 16 years ago

milestone: post-1.0

by jcrocholl, 16 years ago

Attachment: UnsignedIntegerField.diff added

Very simple patch to add UnsignedIntegerField and UnsignedSmallIntegerField as subclasses of PositiveIntegerField and PositiveSmallIntegerField.

comment:4 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:5 by Karen Tracey, 15 years ago

#10719 marked as a dup. I favor its recommendation to explicitly note in the docs these fields include zero as valid input. I don't think we need yet more names for the same thing and it would be backwards-incompatible at this point to start disallowing zero for these fields.

comment:6 by Jeff Anderson, 14 years ago

Owner: changed from Jeff Anderson to nobody
Status: assignednew

comment:7 by jonlesser@…, 13 years ago

I was surprised to get a ZeroDivisionError error when using this field with a ModelForm, because as people have already pointed out, 0 is not a positive integer. So how could that be valid input for the PositiveIntegerField?

Now I'm just specifying a different form field in my ModelForm: forms.IntegerField(min_value=1).

IMO changing the min_value form field default for this field from 0 to 1 is better than making a new UnsignedIntegerField class. At the very least, a caution note in the docs for PositiveIntegerField.

comment:8 by Luke Plant, 13 years ago

Severity: Normal
Type: Bug

comment:9 by Alex Gaynor, 13 years ago

Component: FormsDocumentation
Easy pickings: unset
Summary: PositiveIntegerField and co have a misleading nameDocument that PositiveIntegerField and co have a misleading name
Triage Stage: Design decision neededAccepted
UI/UX: unset

Speaking with Jacob: we can't change this due to backwards compatibility, however the docs should clearly note this.

in reply to:  9 comment:10 by Paul Collins, 13 years ago

Has patch: set

Replying to Alex:

Speaking with Jacob: we can't change this due to backwards compatibility, however the docs should clearly note this.


So it's a trivial patch to the docs, but I guess it makes the required point.

comment:11 by Bas Peschier, 13 years ago

Might it be a good thing to actually mention we know positive should mean "higher than 0", but the field accepts 0 due to backwards compatibility? It saves people a search on the trac for tickets relating to it.

comment:12 by Julien Phalip, 13 years ago

Patch needs improvement: set

Yes, I agree with bpeschier. Could you please update the patch?

comment:13 by Paul Collins, 13 years ago

Should I include that kind of information as a ..note:: or just put something to the effect of

Like an :class:IntegerField, but must be positive. Also accepts 0 for backward compatibility reasons (link to this ticket)?

comment:14 by Paul Collins, 13 years ago

Cc: Paul Collins added

in reply to:  13 ; comment:15 by Julien Phalip, 13 years ago

Replying to paulcollins:

Should I include that kind of information as a ..note:: or just put something to the effect of

Like an :class:IntegerField, but must be positive. Also accepts 0 for backward compatibility reasons (link to this ticket)?

No need to use a "..note" directive. Just need to say that, despite its name, it accepts 0 for B-C reasons.

by Paul Collins, 13 years ago

Attachment: doc_patch.diff added

Update per comment 15

by Paul Collins, 13 years ago

Attachment: doc_patch_wo_link.diff added

Same idea without the link to this ticket

in reply to:  15 comment:16 by Paul Collins, 13 years ago

Owner: changed from nobody to Paul Collins
Patch needs improvement: unset

Replying to julien:

Replying to paulcollins:

No need to use a "..note" directive. Just need to say that, despite its name, it accepts 0 for B-C reasons.

Two versions one with the link to this ticket and one without. Let me know if you need some other change to it, otherwise feel free to use either one :)

comment:17 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

Great, thanks ;)

comment:18 by Julien Phalip, 13 years ago

Resolution: fixed
Status: newclosed

In [16972]:

Fixed #7609 -- Noted in the model fields reference documentation that PositiveIntegerField accepts the value 0 for backwards-compatibility reasons. Thanks to everyone involved in the resolution of this issue, including Paul Collins for the patch.

comment:19 by anonymous, 11 years ago

+1 for UnsignedIntegerField

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