[Django] #35284: PositiveIntegerField description is confusing

[Django]

#35284: PositiveIntegerField description is confusing
------------------------------------------------+------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
In ticket:7609, the description for PositiveIntegerField was changed from
saying that its value "must be positive" to saying it "must be positive or
zero". This is clearly and unarguably an improvement. However the
following sentence was also added: "The value `0` is accepted for backward
compatibility reasons." This additional sentence is inaccurate and
confusing. I say "inaccurate" because zero isn't accepted for "backward
compatibility", it's accepted because that's the behaviour most people
want. I say "confusing" because it makes it sound like using the field to
store zeroes is somehow deprecated and likely to be changed in the future,
which as far as I can see is not the case. This completely unnecessarily
reduces peoples' confidence in using this field type. I suggest simply
removing that sentence.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35284: PositiveIntegerField description is confusing

-------------------------------------+-------------------------------------

Reporter: Jon Ribbens | Owner: nobody

Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

-------------------------------------+-------------------------------------
Comment (by Jon Ribbens):

https://github.com/django/django/pull/17955
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:1>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by bcail):

I don't think it's inaccurate to reference backwards compatibility - it
was [https://code.djangoproject.com/ticket/7609#comment:9 clearly
mentioned] in the previous ticket. But, if the docs make it sound like
users shouldn't store 0 in the field, maybe there's a clarification that
could be added to the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:2>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Ribbens):

I know it was mentioned previously, but that doesn't make it not
inaccurate, which it certainly is. It accepts zero because that's what
people writing code using it ''today'' want it to do. That's not what
"backwards compatibility" means.

I don't see what better clarification or improvement could be made other
than to simply remove the sentence, which is what my pull request does.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:3>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by bcail):

I think some people might be using "PositiveIntegerField" as the best
option they have (although they wish it didn't accept zero), or it might
do what they want, but they still think the name "PositiveIntegerField" is
confusing and/or inaccurate.

Some people (on the previous ticket) thought adding that sentence was
helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:4>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Ribbens):

Hence why I agree that adding the words "or zero" were clearly an
improvement. Possibly I am not explaining myself well enough.

If there is no proposal in contemplation to change the behaviour at some
point in the future so it won't accept zero and hence people writing code
today should avoid using this field type if they wish to store zeroes, the
reference to "backward compatibility" is simply straight-up false.

If there is such a proposal then the wording should be changed instead to
indicate that if people want to store zeroes then they should not use this
field, they should use IntegerField with a customer validator or
something.

Maybe at the time of the previous ticket people were thinking that the
behaviour was going to be changed soon as an inevitable next step, but
that was twelve years ago so if they did think that they were clearly
mistaken.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:5>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by bcail):

I think the reference to backwards compatibility in the docs is an
explanation from the Django developers for why they didn't change
PositiveIntegerField to not accept zero. It started out accepting zero,
and they would change it to not accept zero anymore, but they aren't going
to change it now because code depends on it accepting zero (ie. "backwards
compatibility").
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:6>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

I have a PhD in computer science and a strong background in mathematics,
and believe me the answer for the question "Is 0 a positive number?" is
not obvious :)
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:7>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: Documentation | Version: 5.0

Severity: Normal | Resolution: invalid

Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* resolution: => invalid
* status: new => closed

Comment:

I agree that the sentence "The value 0 is accepted for backward
compatibility reasons." provides a current, valuable and necessary
clarification. Users may likely expect that 0 is not accepted, so having
this information helps manage expectations effectively.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:8>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Ribbens):

I rather think you're missing the point completely - the sentence doesn't
"provide a clarification", it misleads people and makes things less clear,
as I explained in my argument above that nobody has provided a response
to. But obviously I can't force you to improve your product. Oh well.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:9>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

Replying to [comment:9 Jon Ribbens]:

> I rather think you're missing the point completely - the sentence
doesn't "provide a clarification", it misleads people and makes things
less clear, as I explained in my argument above that nobody has provided a
response to. But obviously I can't force you to improve your product. Oh
well.

Hey Jon, I beg to differ. Both bcail and myself are answering to your
argument: while there is ''not'' a proposal to deprecate allowing 0 in the
field, the sentence warns the users about potential unexpected outcomes.
It's easy to assume that a `PositiveIntegerField` would not allow 0, so
the note in the docs is making it clear that 0 ''is allowed'' and it will
be for the foreseeable future due to the backward compatibility goal.

I hope that helps!
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:10>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Ribbens):

Replying to [comment:10 Natalia Bidart]:

> Hey Jon, I beg to differ. Both bcail and myself are answering to your
argument: while there is ''not'' a proposal to deprecate allowing 0 in the
field, the sentence warns the users about potential unexpected outcomes.
It's easy to assume that a `PositiveIntegerField` would not allow 0, so
the note in the docs is making it clear that 0 ''is allowed'' and it will
be for the foreseeable future due to the backward compatibility goal.

Sorry, that just confirms that you're missing what I'm trying to say. As I
said right at the start, the change to explicitly say that zero is allowed
was clearly a good and beneficial change. Everyone is agreed on that. But
the sentence I'm suggesting be removed, which says that this is "for
backward compatibility reasons", ''doesn't'' make it clear that this will
behaviour will remain "for the foreseeable future". That's my entire
point!

Actually it conveys the precise opposite information, it suggests that it
is foreseen that in the future, the behaviour may change so that zero will
not be accepted, and so if you want to store zeroes you should not be
using this field type. It is saying "we don't think this field should
accept zeroes, but we haven't been able to fix it yet because of backwards
compatibility considerations, so that change is still on the to-do list".

That's why I think the wording should change, because it does ''not''
convey the message you think it does (which is not, frankly, a message
that needs conveying in the first place).

Would anyone be interested in a patch to make it so that it was
`PositiveIntegerField(allow_zero=True)` (and similarly for the other two
"Positive" fields)? i.e. it maintains backwards compatibility, but the
programmer can choose whether they want zeroes allowed or not? I might
find that an interesting little project, but only if there was a
reasonable chance the patch would get accepted (I was expecting this one
to be a shoo-in!)
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:11>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia Bidart):

Replying to [comment:11 Jon Ribbens]:
> [...] the sentence I'm suggesting be removed, which says that this is

"for backward compatibility reasons", ''doesn't'' make it clear that this
will behaviour will remain "for the foreseeable future". That's my entire
point!

I fully understand your point, but I disagree. Deprecation notices are
documented quite clearly and they follow a strict procedure of when they
can be introduced and when they occur. So until these docs explicitly say
"Deprecated in Django version X.Y", the value `0` will be allowed for the
foreseeable future.

> Actually it conveys the precise opposite information, it suggests that
it is foreseen that in the future, the behaviour may change so that zero
will not be accepted, and so if you want to store zeroes you should not be
using this field type. It is saying "we don't think this field should
accept zeroes, but we haven't been able to fix it yet because of backwards
compatibility considerations, so that change is still on the to-do list".

I understand that this is how ''you'' read the sentence, but we disagree
on what it means. Saying ''The value 0 is accepted for backward
compatibility reasons.'' is just an **explanation**, is not a heads-up
that is going to be changed (as mentioned before, deprecation notices are
handled quite differently). The sentence only explains **why** it is the
way it is. Otherwise we'd regularly get new tickets saying
"PositiveIntegerField should not accept 0 because 0 is not a positive
number". In order to manage expectations from Django users, the
clarification is there saying "we know this is weird but it's there for a
reason".

> Would anyone be interested in a patch to make it so that it was
`PositiveIntegerField(allow_zero=True)` (and similarly for the other two
"Positive" fields)? i.e. it maintains backwards compatibility, but the
programmer can choose whether they want zeroes allowed or not? I might
find that an interesting little project, but only if there was a
reasonable chance the patch would get accepted (I was expecting this one
to be a shoo-in!)

You are welcomed to share the idea with the Django community, following
the [https://docs.djangoproject.com/en/stable/internals/contributing/bugs-
and-features/#requesting-features the documented guidelines for requesting
features]. I, personally, don't think this is worth adding since by just
using a custom and trivial validator (or just using `MinValueValidator`)
the need of not allowing `0` in the field is solved, and in my experience,
allowing for 0 is the less surprising semantic.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:12>

[Django]

#35284: PositiveIntegerField description is confusing
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jon Ribbens):

Replying to [comment:12 Natalia Bidart]:

> Replying to [comment:11 Jon Ribbens]:

> > Actually it conveys the precise opposite information, it suggests that
it is foreseen that in the future, the behaviour may change so that zero
will not be accepted, and so if you want to store zeroes you should not be
using this field type. It is saying "we don't think this field should
accept zeroes, but we haven't been able to fix it yet because of backwards
compatibility considerations, so that change is still on the to-do list".
>
> I understand that this is how ''you'' read the sentence, but we disagree
on what it means.

Which pretty much by definition means it's unclear, which is what I said
all along. I'm not sure why you want it to remain unclear, but as I said,
I can't force you to improve things.
--
Ticket URL: <https://code.djangoproject.com/ticket/35284#comment:13>