PositiveIntegerField description is confusing

[Jon Ribbens]

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.

[Jon Ribbens]

​https://github.com/django/django/pull/17955

[bcail]

I don't think it's inaccurate to reference backwards compatibility - it was 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.

[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.

[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.

[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.

[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").

[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 :)

[Natalia Bidart]

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.

[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.

[Natalia Bidart]

Replying to 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!

[Jon Ribbens]

Replying to 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!)

[Natalia Bidart]

Replying to 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 ​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.

[Jon Ribbens]

Replying to Natalia Bidart:

Replying to 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.