Opened 7 years ago

Closed 4 years ago

Last modified 2 years ago

#7609 closed Bug (fixed)

Document that PositiveIntegerField and co have a misleading name

Reported by: Gergely Kontra <pihentagy+djangoproject@…> Owned by: paulcollins
Component: Documentation Version: master
Severity: Normal Keywords: name convention, rename
Cc: paulcollins 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 7 years ago.
Very simple patch to add UnsignedIntegerField and UnsignedSmallIntegerField as subclasses of PositiveIntegerField and PositiveSmallIntegerField.
doc_patch.diff (600 bytes) - added by paulcollins 4 years ago.
Update per comment 15
doc_patch_wo_link.diff (519 bytes) - added by paulcollins 4 years ago.
Same idea without the link to this ticket

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by adamv

  • 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 7 years ago by programmerq

  • Owner changed from nobody to programmerq
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Design 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 7 years ago by julianb

  • milestone set to post-1.0

Changed 7 years ago by jcrocholl

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

comment:4 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:5 Changed 6 years ago by kmtracey

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

  • Owner changed from programmerq to nobody
  • Status changed from assigned to new

comment:7 Changed 4 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 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:9 follow-up: Changed 4 years ago by Alex

  • Component changed from Forms to Documentation
  • Easy pickings unset
  • Summary changed from PositiveIntegerField and co have a misleading name to Document that PositiveIntegerField and co have a misleading name
  • Triage Stage changed from Design decision needed to Accepted
  • 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 4 years ago by paulcollins

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

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

  • Patch needs improvement set

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

comment:13 follow-up: Changed 4 years ago by 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)?

comment:14 Changed 4 years ago by paulcollins

  • Cc paulcollins added

comment:15 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by julien

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

Update per comment 15

Changed 4 years ago by paulcollins

Same idea without the link to this ticket

comment:16 in reply to: ↑ 15 Changed 4 years ago by paulcollins

  • Owner changed from nobody to paulcollins
  • 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 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Great, thanks ;)

comment:18 Changed 4 years ago by julien

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

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

+1 for UnsignedIntegerField

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