Better nullability definitions for all generators #419
Conversation
|
@agronholm can you please have a look? I'd like to release 3.1 after this is merged. |
| if is_primary: | ||
| kwargs["primary_key"] = True | ||
| if not column.nullable and not is_sole_pk and is_table: | ||
| if not column.nullable and not column.primary_key: |
There was a problem hiding this comment.
It is possible for some columns of a multi-column primary key constraint to be NULL, though this is exceedingly rare in practice. But this is why I had the condition in place. Perhaps we could simply drop the is_table condition?
There was a problem hiding this comment.
Hmmmm. I do understand this concern. However, this resulted in a somewhat weird behavior where some composite primary keys columns were getting the nullability flag. Maybe I misunderstood what I was seeing. I'll return this flag and check again
There was a problem hiding this comment.
I'm not sure we have a test covering the case of partially nullable PK, but it might be worth adding one if we don't have it already.
There was a problem hiding this comment.
Ok, I realized what was bugging me. I've improved the conditioning and added a test to cover exactly the case you mentioned
sheinbergon
force-pushed
the
sheinbergon/explicit_nullability_definition
branch
from
3756a3c to
8b6794b
Compare
Fixes #412
Looks like both
declarativeandsqlmodelsgenerators were omitting non-nullability from the column definitionChecklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) which would fail without your patchCHANGES.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.