Fixed #28275 -- Added more hooks to SchemaEditor._alter_field(). #8487
Conversation
django/db/backends/base/schema.py
Outdated
| new_field.get_internal_type() in ("CharField", "TextField")): | ||
| # The field is nullable in the database anyway, leave it alone | ||
| pass | ||
| elif new_field.null: |
There was a problem hiding this comment.
I think that we can keep this more DRY, i.e.:
else:
sql = self.sql_alter_column_null if new_field.null else self.sql_alter_column_not_null
return (
sql % {
"column": self.quote_name(new_field.column),
"type": new_type,
},
[],
)
django/db/backends/base/schema.py
Outdated
| @@ -407,14 +407,12 @@ def add_field(self, model, field): | |||
| # Drop the default if we need to | |||
| # (Django usually does not use in-database defaults) | |||
| if not self.skip_default(field) and self.effective_default(field) is not None: | |||
| sql, params = self._alter_column_default_sql(model, None, field, True) | |||
There was a problem hiding this comment.
Use drop=True rather than an arg for better readability.
Use a different var for sqlhere, maybe changes_sql? It's a little confusing to have the sql name reused in the next line.
django/db/backends/base/schema.py
Outdated
| @@ -767,19 +726,70 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, | |||
| # Drop the default if we need to | |||
| # (Django usually does not use in-database defaults) | |||
| if needs_database_default: | |||
| sql, params = self._alter_column_default_sql(model, old_field, new_field, True) | |||
There was a problem hiding this comment.
Use drop=True rather than an arg for better readability.
Use a different var for sqlhere, maybe changes_sql? It's a little confusing to have the sql name reused in the next line.
django/db/backends/base/schema.py
Outdated
| def _alter_column_type_sql(self, table, old_field, new_field, new_type): | ||
| def _alter_column_null_sql(self, model, old_field, new_field): | ||
| """ | ||
| Hook to specialize column null alteration for different backends. |
There was a problem hiding this comment.
I'd say "Return a (sql, params) fragment to set a column to null or non-null as required by new_field, or None if no changes are required."
django/db/backends/base/schema.py
Outdated
|
|
||
| Return a sql fragment (sql, params). | ||
| """ | ||
| new_db_params = new_field.db_parameters(connection=self.connection) |
There was a problem hiding this comment.
Move this to the else block since it's only used there?
django/db/backends/base/schema.py
Outdated
|
|
||
| def _alter_column_default_sql(self, model, old_field, new_field, drop=False): | ||
| """ | ||
| Hook to specialize column default alteration for different backends. |
There was a problem hiding this comment.
I'd say, "Return a (sql, params) fragment to add or drop (depending on the drop argument) a default to new_field's column."
django/db/backends/base/schema.py
Outdated
| """ | ||
| new_default = self.effective_default(new_field) | ||
| new_db_params = new_field.db_parameters(connection=self.connection) | ||
|
|
There was a problem hiding this comment.
I'd drop the blank line here and 2 lines below.
django/db/backends/base/schema.py
Outdated
| Return a sql fragment (sql, params). | ||
| """ | ||
| new_default = self.effective_default(new_field) | ||
| new_db_params = new_field.db_parameters(connection=self.connection) |
There was a problem hiding this comment.
This could be moved closer to where it's used or even eliminated as an intermediate variable.
django/db/backends/base/schema.py
Outdated
| elif self.connection.features.requires_literal_defaults: | ||
| # Some databases can't take defaults as a parameter (oracle) | ||
| # If this is the case, the individual schema backend should | ||
| # implement prepare_default |
timgraham
changed the title
timgraham
force-pushed
the
alter_field_cleanups
branch
from
1044ee7
to
6d3e2bb
Compare
timgraham
changed the title
Note: As annoying as the latter is, the streamlining is kinda required
to have the model everywhere as opposed to just the table name.