Well done. Shows either you're a very intuitive dev, or Django is a very intuitive tool. Maybe both.
These problems I think are more to do with inertia and not having best practices in mind. It's easy to do CharField and not thing about TextField.
Plus over time requirements change, so when a model was created maybe the kwargs were perfectly valid - but over time as refactors occurred tech debt crept in.
This makes me curious; wouldn't it be possible to write an automated linter or static code analyzer that would catch and automatically fix these issues?
In Node environment, I think that I would be able to come up with an ESLint plugin. Doesn't Python ecosystem have something similar?
I think not because a think the bot cannot know is "why". An anti-pattern may be done for solid business reasons. It's Django Doctor's job to say "x could be y" and it's the dev's job to approve/reject the advice based on what the dev knows about the business and problems being solved.
> I also don't see anything wrong with explicit default arguments.
I think explicit default arguments should be used when they're documenting some behavior that the reader might not know or remember, but that isn't the case here.
E.g. with python's enumerate built-in, it's useful to the reader to know that it starts at 0 or whatever. But with Django's models, adding (null=False, blank=False, editable=True) isn't telling the reader anything that they likely wouldn't already know or assume, it's just making the code more difficult to read.
Yes, I first read those as (null=True, blank=True) because I've never written out those arguments when their default is False. So for me writing them out explicitly with their defaults is an anti-pattern.
I really hate how Django doesn’t care about the difference between null (the lack of a value) and the empty string (the presence of a specific value). But if you’re going to use HTML forms as a way of editing that data in a generic way, I don’t really see a better option. How would you present a form containing a text field that was null in such a way that it would be preserved after the user edits a different field and saves?
You should probably check the data coming from the form and what's in the database. If the form sends an empty string for a null field in the database, you keep the null. If it sends an empty string for a not null field, you set an empty string in the db field. This assumes that the semantic of null is "never touched before".
You use hidden fields in the html form to preserve previous values and then compare them in your input validation code server side. You set the default value of input type text and text area to the previous value. If the value was blank before and after you save as null, otherwise you set it to the new value.
In the real world the GitHub PR bot gives the dev the choice of committing the advice or not. I appreciate that does not reflect perfectly in the game :)
> I also don't see anything wrong with explicit default arguments.
I have worked on similar projects to this game in the past (at scale) so it's close to my heart.
To me the benefit of the default redundant arguments error is pedagogical. It forces me to think more closely about what it means to set nullable, assuming I was coming to the game with a limited understanding of Django code. Just glimpsing a note in the docs doesn't register nearly as quickly interactive play does.
I would love to see this format used more extensively. When I'm learning an API I tend to fire up a REPL and explore it by poking. I do it even with basic mathematics that have fallen out of my brain.
Very much enjoyed and it served as a quick refresher!
I think for code readability show defaults can be confusing, unless you _always_ show those defaults.
The following implies that bar might be null-able if you don't know what the default is.
foo = text(null=False)
bar = text()
While writing code you likely have the defaults you care about in your head. Someone reading your code may not. You want to make sure your code leads their thought process down the right path. So either you _always_ show the default, or you only override the default.
Where does the `@sql` decorator come from? If it's not adding attributes that are django specific, then you lose most of django's functionality built around models: automatic admin, user model, etc...
Just a thought when it might be viable: if a subset of that table receives a lot of updates, and it would be helpful to your access patterns if some of it was separated into a different table that was rarely updated but accessed a lot?
Here’s a good example of 1:1 denormalization: Say I’m running a music streaming service. I can store my music tracks as a giant database of MP3s, ID3 tags included, and parse out each ID3 tag on each read. I could, alternatively, store the music tracks as a giant database of music tracks, which has “Artist”, “Album”, etc. fields, and one field for “MP3” that contains no id3 tags. Both are “normalized” data structures. Storing the id3 tags with the MP3 would be denormalization. It might be much saner to do that than to reconstruct them every time someone goes to download them.
For performance, though - You want to store the MP3s in their own table, or not even in a “table” but as a blobstore. Storing the MP3 files for your tracks inline is entirely feasible. 3-10MB inline blobs will work in your modern database engine of choice, and simplify a lot - But you’ll often want to write your presentation layer with “SELECT * FROM tracks” - And you can do that if you just store the data blob separately; i.e. denormalize.
If you have hundreds of fields on a record, selecting from the subset that are actually relevant is far, far saner. Particularly when a large group can be nullable, and will likely be nullable together. If 10% of your business is washers - Washers of every shape, thickness, hole-size, material, etc. - You will have all of those attributes in your parts database. It’s best to treat those as first-class citizens, but they belong in a nullable 1:1 relationship.
Very fun. This is the form of studying I enjoy the most - by correcting "other's mistakes" (and getting hints when I can't). I actually believe it is very efficient. I wish there were more games like this, for all the subjects imaginable.
If I'm on postgres and actually want my comments to have a max length of 5000, is it suggesting that using CharField(max_length=5000) is a problem with my design? Using CharField / VARCHAR vs TextFIeld / TEXT should not have a performance difference in postgresql.
Thanks for replying, and yes that is the page about which I have questions.
* The error code C2006 assumes there is a clear divide between "smaller" and "larger" strings. What is smaller, and what is larger? Is 1000 large? Is 5000 large? Is 1000000 large? If I want my comment to be capped at 5000 characters to encourage healthy discussions but prevent spamming, is it large?
* If a field has no "known format" and that's where it draws the line between CharField and TextField, then why is the error C2006 still raised when TextField(max_length=5000) is used? Why does the validator recommend allowing anyone to insert entries of unlimited size?
I don't know about Django's ORM but normally I would use a TEXT field and then set a separate char_length CONSTRAINT on the field.
I think it's important to have the length constraint at the database level for data consistency's sake. Then you can choose to add in app level validations to make more friendly user error messages.
Changing a constraint's length is also a much easier database operation vs. changing a varchar's length.
A nice advantage of having a TextField with a check constraint, instead of a varchar, is that the locks required in Postgres to change the check constraint are less extreme than the ones required to change the length of varchar.
With a check constraint, we can use `NOT VALID` with a separate `VALIDATE` call, while a change to the varchar length requires an `ACCESS EXCLUSIVE` lock.
I think what they are trying to say, is that if you have a huge max_length it is possibly because you accept any type of text, and you felt the number was high enough.
If you use postgres, you want to restrict the length (even if it is 5000) ? => CharField, you don't care about the length => TextField
Please make a (fully fledged) out of this. As a Django user (and big fan), I enjoyed playing this little game and would love it too spend much time on it. Plus if would be a great way to learn new stuff.
54 comments
[ 4.6 ms ] story [ 115 ms ] threadDjango Doctor is a code improvement bot you can add to GitHub to review your PRs
- Django is big (because it solves a lot of problems)
- there exists a learning curve
Sometimes people miss best practices when the interplay between those two points occur.
Fortunately bots exist to help with that problem
Lint tools are usually overly picky but this seems like a good choice of issues
Are those problems really about Django the framework and not about data modeling in relational databases?
These problems I think are more to do with inertia and not having best practices in mind. It's easy to do CharField and not thing about TextField.
Plus over time requirements change, so when a model was created maybe the kwargs were perfectly valid - but over time as refactors occurred tech debt crept in.
In Node environment, I think that I would be able to come up with an ESLint plugin. Doesn't Python ecosystem have something similar?
This is why linters let you ignore lines/files.
I think explicit default arguments should be used when they're documenting some behavior that the reader might not know or remember, but that isn't the case here.
E.g. with python's enumerate built-in, it's useful to the reader to know that it starts at 0 or whatever. But with Django's models, adding (null=False, blank=False, editable=True) isn't telling the reader anything that they likely wouldn't already know or assume, it's just making the code more difficult to read.
> I also don't see anything wrong with explicit default arguments.
Very much your and your team's choice. I for one see them as noise. Does it add value explicitly stating all the fields of Field? https://docs.djangoproject.com/en/3.1/ref/models/fields/#fie...
If you would not explicitly state all of the defaults why state a subset of them?
It's "have you considered" rather than "you must do this".
The dev has the choice of if to commit the change that is suggested
That gets a bit lost in the game I know :)
To me the benefit of the default redundant arguments error is pedagogical. It forces me to think more closely about what it means to set nullable, assuming I was coming to the game with a limited understanding of Django code. Just glimpsing a note in the docs doesn't register nearly as quickly interactive play does.
I would love to see this format used more extensively. When I'm learning an API I tend to fire up a REPL and explore it by poking. I do it even with basic mathematics that have fallen out of my brain.
Very much enjoyed and it served as a quick refresher!
The following implies that bar might be null-able if you don't know what the default is.
While writing code you likely have the defaults you care about in your head. Someone reading your code may not. You want to make sure your code leads their thought process down the right path. So either you _always_ show the default, or you only override the default.https://github.com/ppinard/dataclasses-sql/issues/4
My belief is that the django functionality can be added back in the form of decorators.
I have similar work to add @graphql to python dataclasses.
Looks cool.
Never understood that advice, what's the point of complexifying your model because a class has too many attributes?
Also makes it easier to understand by giving a set of fields a name, sort of like a summary.
For performance, though - You want to store the MP3s in their own table, or not even in a “table” but as a blobstore. Storing the MP3 files for your tracks inline is entirely feasible. 3-10MB inline blobs will work in your modern database engine of choice, and simplify a lot - But you’ll often want to write your presentation layer with “SELECT * FROM tracks” - And you can do that if you just store the data blob separately; i.e. denormalize.
If you have hundreds of fields on a record, selecting from the subset that are actually relevant is far, far saner. Particularly when a large group can be nullable, and will likely be nullable together. If 10% of your business is washers - Washers of every shape, thickness, hole-size, material, etc. - You will have all of those attributes in your parts database. It’s best to treat those as first-class citizens, but they belong in a nullable 1:1 relationship.
If I'm on postgres and actually want my comments to have a max length of 5000, is it suggesting that using CharField(max_length=5000) is a problem with my design? Using CharField / VARCHAR vs TextFIeld / TEXT should not have a performance difference in postgresql.
* The error code C2006 assumes there is a clear divide between "smaller" and "larger" strings. What is smaller, and what is larger? Is 1000 large? Is 5000 large? Is 1000000 large? If I want my comment to be capped at 5000 characters to encourage healthy discussions but prevent spamming, is it large?
* If a field has no "known format" and that's where it draws the line between CharField and TextField, then why is the error C2006 still raised when TextField(max_length=5000) is used? Why does the validator recommend allowing anyone to insert entries of unlimited size?
I think it's important to have the length constraint at the database level for data consistency's sake. Then you can choose to add in app level validations to make more friendly user error messages.
Changing a constraint's length is also a much easier database operation vs. changing a varchar's length.
With a check constraint, we can use `NOT VALID` with a separate `VALIDATE` call, while a change to the varchar length requires an `ACCESS EXCLUSIVE` lock.
If you use postgres, you want to restrict the length (even if it is 5000) ? => CharField, you don't care about the length => TextField
It's genuinely mind-boggling why, after 15 years, Django still suffers from incompletions like this.
Neat demo tho !
I had 23 out of 24 issues, just a few minutes ago
Future version of the quiz will close that bug :)