4 ms·

Empty string and none are not the same or in any way redundant. Linters belong in ci. Humans belong in code review. I can actually explain to a human why my te

Empty string and none are not the same or in any way redundant. Linters belong in ci. Humans belong in code review. I can actually explain to a human why my text field has a default of none.

swiley 6y ago
I hate things that get this wrong so much.
- rikatee 6y ago
  I'm sorry swiley :'(
VWWHFSfQ 6y ago
It's a code-smell in Django, but I agree that this kind of thing shouldn't be "auto-fixed" by a bot. There are perfectly valid reasons why you might want to eschew the convention. > Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data;” the Django convention is to use the empty string, not NULL. One exception is when a CharField has both unique=True and blank=True set. In this situation, null=True is required to avoid unique constraint violations when saving multiple objects with blank values. [0] https://docs.djangoproject.com/en/3.1/ref/models/fields/#null https://docs.djangoproject.com/en/3.1/ref/models/fields/#nul...
- rikatee 6y ago
  Perhaps I need to clarify "auto fix" - the dev still needs to click "commit" on the suggestion. Given a PR with 3 Django Doctor suggestions, 2 could be ignored and only 1 committed by the dev if they choose.
- eli 6y ago
  Sure, but if it's outside the usual convention it should be explained with a comment. The comment can also include a command to tell the linter to ignore it.
- theptip 6y ago
  Unfortunately it's tricky to add new fields in a zero-downtime fashion without making them nullable. Django does not store defaults in the DB (this is surprising, and the opposite of the Rails approach), and this means that if you upgrade your DB first, until you've upgraded your application all writes to your model are going to fail because the (down-level) application code doesn't know it has to specify the (up-level) DB field. From chatting to folks, most applications just YOLO this, and accept downtime on every such DB operation; you might not even notice that this is happening at first because you'd need to have a create _during_ your DB migration. It'll bite you sooner or later though. I'm sure for many apps a client-side retry is totally acceptable, assuming it's safe to do so.
  - rikatee 6y ago
    If anyone would like to read more on the topic I wrote a blog post that includes this point and others: https://djangodoctor.medium.com/the-real-difference-between-blank-true-and-null-true-may-surprise-you-982a9ac3b9da https://djangodoctor.medium.com/the-real-difference-between-... because while Django Doctor suggests potential improvements, I acknowledge they're not guaranteed to be correct according to the context
- ninjapenguin54 6y ago
  I've read the django docs. It's dumb advice parading as "best practice". The idea that empty string and none are equivalent is distasteful. Nobody makes this argument for int fields right? "Just use 0"? I hate having different conventions for different field types and losing a potentially meaningful distinction between empty string and 0. The idea seems to be born out of truthy & falsey values. (A big source of gotchas and bugs in my experience) I'd say it's the role of django forms to clean data into a normalized form. So converting null to empty string there might make sense.
tmarice 6y ago
In most cases they are synonymous. If they're not, you are allowed to disregard the bot's suggestion.
rikatee 6y ago
Bear in the comments are "food for thought" and aren't aimed at blocking merge. In the review you can pick which suggestions you want and can ignore the stuff that does not add value to you. Soon we will be adding a config file so you can mute the things that ar vexatious
deleted 6y ago
[deleted]

Empty string and none are not the same or in any way redundant. Linters belong in ci. Humans belong in code review. I can actually explain to a human why my text field has a default of none.

swiley 6y ago

I hate things that get this wrong so much.

rikatee 6y ago
I'm sorry swiley :'(

VWWHFSfQ 6y ago

It's a code-smell in Django, but I agree that this kind of thing shouldn't be "auto-fixed" by a bot. There are perfectly valid reasons why you might want to eschew the convention. > Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data;” the Django convention is to use the empty string, not NULL. One exception is when a CharField has both unique=True and blank=True set. In this situation, null=True is required to avoid unique constraint violations when saving multiple objects with blank values. [0] https://docs.djangoproject.com/en/3.1/ref/models/fields/#null https://docs.djangoproject.com/en/3.1/ref/models/fields/#nul...

rikatee 6y ago
Perhaps I need to clarify "auto fix" - the dev still needs to click "commit" on the suggestion. Given a PR with 3 Django Doctor suggestions, 2 could be ignored and only 1 committed by the dev if they choose.
eli 6y ago
Sure, but if it's outside the usual convention it should be explained with a comment. The comment can also include a command to tell the linter to ignore it.
theptip 6y ago
Unfortunately it's tricky to add new fields in a zero-downtime fashion without making them nullable. Django does not store defaults in the DB (this is surprising, and the opposite of the Rails approach), and this means that if you upgrade your DB first, until you've upgraded your application all writes to your model are going to fail because the (down-level) application code doesn't know it has to specify the (up-level) DB field. From chatting to folks, most applications just YOLO this, and accept downtime on every such DB operation; you might not even notice that this is happening at first because you'd need to have a create _during_ your DB migration. It'll bite you sooner or later though. I'm sure for many apps a client-side retry is totally acceptable, assuming it's safe to do so.
- rikatee 6y ago
  If anyone would like to read more on the topic I wrote a blog post that includes this point and others: https://djangodoctor.medium.com/the-real-difference-between-blank-true-and-null-true-may-surprise-you-982a9ac3b9da https://djangodoctor.medium.com/the-real-difference-between-... because while Django Doctor suggests potential improvements, I acknowledge they're not guaranteed to be correct according to the context
ninjapenguin54 6y ago
I've read the django docs. It's dumb advice parading as "best practice". The idea that empty string and none are equivalent is distasteful. Nobody makes this argument for int fields right? "Just use 0"? I hate having different conventions for different field types and losing a potentially meaningful distinction between empty string and 0. The idea seems to be born out of truthy & falsey values. (A big source of gotchas and bugs in my experience) I'd say it's the role of django forms to clean data into a normalized form. So converting null to empty string there might make sense.

tmarice 6y ago

In most cases they are synonymous. If they're not, you are allowed to disregard the bot's suggestion.

rikatee 6y ago

Bear in the comments are "food for thought" and aren't aimed at blocking merge. In the review you can pick which suggestions you want and can ignore the stuff that does not add value to you. Soon we will be adding a config file so you can mute the things that ar vexatious

deleted 6y ago

[deleted]