[Django] #25917: Confusing sentence in RemoveField's documentation
Django
------------------------------------------------+------------------------
Reporter: bmispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
The documentation for `RemoveField` [1] states that:
> if the field is not nullable this may make this operation irreversible
(apart from any data loss, which of course is irreversible)
I find this sentence confusing.
The note in parentheses implies opposition with the previous statement
when it's in fact saying the same thing: the operation is irreversible.
[1] https://docs.djangoproject.com/en/dev/ref/migration-
operations/#django.db.migrations.operations.RemoveField
--
Ticket URL: <https://code.djangoproject.com/ticket/25917>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bmispelon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by rfleschenberg):
As I understand it, the sentence means this: if the field is not nullable,
the reverse of RemoveField (adding a field) may fail, because Django does
not know which data to add for existing rows.
Apart from that, the forward operation (removing the field) is of course
never reversible in so far as it deletes the data in the field.
Question: would RemoveField be reversible if the field has a default
value?
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:1>
Django
Reporter: bmispelon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by bmispelon):
If the field is not nullable, is there a case where the reverse of
`RemoveField` would not fail? If not, the use of "may" is confusing.
As for the question about `default`, that's a good point. I haven't tried
and it could make the reverse operation "work" in the sense that Django
would be able to get back to a valid state but there's no guarantee that
your data would be preserved (any non-default values for that column would
be gone).
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:2>
Django
Reporter: bmispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:3>
Django
Reporter: bmispelon | Owner:
Type: | andersonresende
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by andersonresende):
* owner: nobody => andersonresende
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:4>
Django
Reporter: bmispelon | Owner:
Type: | andersonresende
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by MarkusH):
If I see it correctly, unless Django has a value to put into a restored
column from a previous migration state (e.g. default, null) `RemoveField`
is irreversible.
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:5>
Django
Reporter: bmispelon | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by andersonresende):
* owner: andersonresende =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:6>
Django
Reporter: bmispelon | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by kaifeldhoff):
It seems like everything is pretty clear here and this ticket could be
finished. My suggestion for the respective section in the docs is below.
As I am not a native speaker, I appreciate feedback.
By the time everything is fine, I'd like to add this to the docs being my
first ticket.
''Bear in mind that when reversed this is actually adding a field to a
model. To make this possible, Django needs a default value to populate the
recreated empty column. This is only possible for a field that is nullable
or has a defined default value.
On the other hand if the field is not nullable and does not have a default
value, this operation is irreversible (apart from any data loss, which of
course is irreversible).''
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:7>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by kaifeldhoff):
* owner: => kaifeldhoff
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:8>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by kaifeldhoff):
* Attachment "25917.diff" added.
Based on e47f0c1 [1.9.x] Fixed #26136 ...
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by kaifeldhoff):
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:9>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_tests: 1 => 0
Comment:
Are you able to submit the patch as a pull request? That makes review
easier. Please wrap documentation at 79 characters too.
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:10>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Component: Documentation | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"55481bcdeef43ef5e345f8ea3bae87f4a8ec7bb8" 55481bcd]:
{{{
#!CommitTicketReference repository=""
revision="55481bcdeef43ef5e345f8ea3bae87f4a8ec7bb8"
Fixed #25917 -- Clarified reversibility of RemoveField.
Thanks kaifeldhoff for the draft patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:11>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6103b4990025656a649caad641abb135941f6bf4" 6103b499]:
{{{
#!CommitTicketReference repository=""
revision="6103b4990025656a649caad641abb135941f6bf4"
[1.9.x] Fixed #25917 -- Clarified reversibility of RemoveField.
Thanks kaifeldhoff for the draft patch.
Backport of 55481bcdeef43ef5e345f8ea3bae87f4a8ec7bb8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:12>
Django
Reporter: bmispelon | Owner:
Type: | kaifeldhoff
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by kaifeldhoff):
At least it helped!
I'm having trouble to get Sphinx to work, so I was still unsure about the
effect of wrapping the lines.
--
Ticket URL: <https://code.djangoproject.com/ticket/25917#comment:13>