Opened 8 years ago

Closed 5 years ago

Last modified 4 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: master
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 8 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 5 years ago.
Update per comment 15
doc_patch_wo_link.diff (519 bytes) - added by Paul Collins 5 years ago.
Same idea without the link to this ticket

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by Adam Vandenberg

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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 Changed 8 years ago by Jeff Anderson

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 Changed 8 years ago by Julian Bez

milestone: post-1.0

Changed 8 years ago by jcrocholl

Attachment: UnsignedIntegerField.diff added

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

comment:4 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:5 Changed 7 years ago by Karen Tracey

#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 Changed 7 years ago by Jeff Anderson

Owner: changed from Jeff Anderson to nobody
Status: assignednew

comment:7 Changed 5 years ago by jonlesser@…

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 Changed 5 years ago by Luke Plant

Severity: Normal
Type: Bug

comment:9 Changed 5 years ago by Alex Gaynor

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.

comment:10 in reply to:  9 Changed 5 years ago by Paul Collins

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 Changed 5 years ago by Bas Peschier

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 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

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

comment:13 Changed 5 years ago by Paul Collins

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 Changed 5 years ago by Paul Collins

Cc: Paul Collins added

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

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.

Changed 5 years ago by Paul Collins

Attachment: doc_patch.diff added

Update per comment 15

Changed 5 years ago by Paul Collins

Attachment: doc_patch_wo_link.diff added

Same idea without the link to this ticket

comment:16 in reply to:  15 Changed 5 years ago by Paul Collins

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 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

Great, thanks ;)

comment:18 Changed 5 years ago by Julien Phalip

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

+1 for UnsignedIntegerField

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