Opened 4 years ago
Closed 10 months ago
#31700 closed New feature (fixed)
Highlight destructive operations in makemigrations output
Reported by: | Tom Forbes | Owned by: | Amir Karimi |
---|---|---|---|
Component: | Migrations | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Makemigrations should highlight destructive and potentially destructive operations in the console output. We advise that makemigrations is only a code generator people sometimes apply them without looking. Highlighting dangerous operations in the output will make them stand out, and might help them notice issues before they blindly apply.
For example:
$ ./manage.py makemigrations ... Migrations for 'some_app': some_app/migrations/0002_delete.py - Delete model SomeModel + Add model AnotherModel ~ Alter field X on Y
We can use -
as a prefix for known destructive operations, +
for "safe" operations and ~
for possibly-destructive operations, colour coded as red, green and yellow respectively.
Mailing list discussion: https://groups.google.com/d/msg/django-developers/A0m8YkPKpZo/Yx8l2P8EAQAJ
Attachments (1)
Change History (28)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|
comment:2 by , 4 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:3 by , 4 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
A similar example of a dangerous migration with a rename involving verbose_name
has come up in #32206 and #24157. That looks like a case where a warning would come in handy.
Tobias McNulty had a suggestion on the mailing list thread:
I'm +1 to doing *something.* Absent a louder reminder, I think it's unrealistic to expect everyone to read the output of makemigrations all the time.
As others have said, I'm not sure
manage.py migrate
is the right time. I think it's too late. The code may have already been committed, who knows what machine the migrations are being run on, etc.
During makemigrations, on the other hand, I don't see anything wrong with formatted text or +/-, but I might go a step further. We already prompt for things like renames and merges. Why not forcibly gain user acceptance before creating a migration that deletes something? ...
Let's take this as an opportunity to give users better warnings to check twice here.
comment:4 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 4 years ago
Looking at the implementation for this output, it feels like the +
for addition, -
for deletion, and ~
for modification approach might be easier to accomplish. Could potentially add a prefix based on the "operation type".
Sounds like all operations follow this same structure of (Add|Remove|Alter){suffix}
so I could possibly use that to identify what type of prefix I'd be using. The other way would be explicitly configuring an operation type in each operation.
Adding colors to each operation might be a little too much, but I like the warning approach in the end (and maybe that can be colored somehow).
What do you guys think?
comment:8 by , 2 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Not sure, I'll deassign myself so you can take this one if you have a patch.
comment:9 by , 2 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:10 by , 2 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:11 by , 16 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
I'd like to work on this issue. I have implemented it in the +/-/~ way. I can add coloring too. I'll create its PR soon.
comment:12 by , 15 months ago
I have prepared the feature. I wasn't sure coloring as highlighting is a good idea so I added both prefixes (+,-,~) and colors.
follow-up: 14 comment:13 by , 14 months ago
Has patch: | set |
---|
Amir, in order for the PR to appear in the review queue, the flag "has patch" needs to be set (I'm setting it now). There are more details about how to proceed with PRs and review feedback in [https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-style this link[.
comment:14 by , 14 months ago
Replying to Natalia Bidart:
Amir, in order for the PR to appear in the review queue, the flag "has patch" needs to be set (I'm setting it now). There are more details about how to proceed with PRs and review feedback in [https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-style this link[.
Thnx Natalia for your notice. As it's kinda new feature, I just wanted to add some docs about it, but I didn't find any post related to such a feature that might be better to consider the community's opinion on the Django forum. I wanna add a new topic about such a feature, and find out if people see it as interesting or noisy.
comment:15 by , 14 months ago
Patch needs improvement: | set |
---|
Marking as "needs improvement" per Adam's comment.
comment:16 by , 13 months ago
Patch needs improvement: | unset |
---|
comment:17 by , 13 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:18 by , 13 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:19 by , 12 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:20 by , 12 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:21 by , 12 months ago
Patch needs improvement: | set |
---|
comment:22 by , 12 months ago
Patch needs improvement: | unset |
---|
comment:23 by , 11 months ago
Cc: | added |
---|
comment:24 by , 11 months ago
Patch needs improvement: | set |
---|
comment:25 by , 11 months ago
Patch needs improvement: | unset |
---|
comment:26 by , 10 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
I'm not sure about this: seems like visual noise for (I suspect) little benefit. I commented on the mailing list discussion: let's see how that goes.