[Django] #31700: Highlight destructive operations in makemigrations output
Django
---------------------------------------+------------------------
Reporter: Tom Forbes | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+------------------------
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 green, red and yellow respectively.
Mailing list discussion: https://groups.google.com/d/msg/django-
developers/A0m8YkPKpZo/Yx8l2P8EAQAJ
--
Ticket URL: <https://code.djangoproject.com/ticket/31700>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tom Forbes | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Tom Forbes:
Old description:
> 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 green, red and yellow respectively.
>
> Mailing list discussion: https://groups.google.com/d/msg/django-
> developers/A0m8YkPKpZo/Yx8l2P8EAQAJ
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:1>
Django
Reporter: Tom Forbes | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:2>
Django
Reporter: Tom Forbes | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: closed => new
* resolution: needsinfo =>
* stage: Unreviewed => Accepted
Comment:
A similar example of a dangerous migration with a rename involving
`verbose_name` has come up in #32206. That looks like a case where a
warning would come in handy.
[https://groups.google.com/g/django-
developers/c/A0m8YkPKpZo/m/elJVb9JCAgAJ 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:3>
Django
Reporter: Tom Forbes | Owner: Modestas Jakuska
Type: New feature | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Modestas Jakuska):
* cc: Modestas Jakuska (added)
* owner: nobody => Modestas Jakuska
* status: new => assigned
Old description:
> 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
New description:
Hey, so I'm totally new to this ('contributing to open source, not Django
or coding). Can I just take this? Seems easy enough for a first Pull
Request :)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:4>
Django
Reporter: Tom Forbes | Owner: Modestas Jakuska
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Modestas Jakuska:
Old description:
> Hey, so I'm totally new to this ('contributing to open source, not Django
> or coding). Can I just take this? Seems easy enough for a first Pull
> Request :)
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:5>
Django
Reporter: Tom Forbes | Owner: (none)
Type: New feature | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Modestas Jakuska):
* cc: Modestas Jakuska (removed)
* owner: Modestas Jakuska => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:6>
Django
Reporter: Tom Forbes | Owner: Ramon Saraiva
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ramon Saraiva):
* owner: nobody => Ramon Saraiva
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:4>
Django
Reporter: Tom Forbes | Owner: Ramon Saraiva
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ramon Saraiva):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:5>
Django
Reporter: Tom Forbes | Owner: Ramon Saraiva
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
Anyone working on this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:6>
Django
Reporter: Tom Forbes | Owner: Ramon Saraiva
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* Attachment "colored-ops.jpg" added.
Colored migration operations
Django
Reporter: Tom Forbes | Owner: Ramon Saraiva
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
should i prepare a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:7>
Django
Reporter: Tom Forbes | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ramon Saraiva):
* owner: Ramon Saraiva => (none)
* status: assigned => new
Comment:
Not sure, I'll deassign myself so you can take this one if you have a
patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:8>
Django
Reporter: Tom Forbes | Owner: Bhuvnesh
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* owner: (none) => Bhuvnesh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:9>
Django
Reporter: Tom Forbes | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bhuvnesh):
* owner: Bhuvnesh => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:10>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Amir Karimi):
* owner: (none) => Amir Karimi
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:11>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Amir Karimi):
I have prepared the feature. I wasn't sure coloring as highlighting is a
good idea so I added both prefixes (+,-,~) and colors.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:12>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* has_patch: 0 => 1
Comment:
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[.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:13>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Amir Karimi):
Replying to [comment:13 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:14>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
Comment:
Marking as "needs improvement" per [https://forum.djangoproject.com/t
/makemigrations-highlighter-interesting-or-noise/24039/6 Adam's comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:15>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Amir Karimi):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:16>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:17>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Amir Karimi):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:18>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:19>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Amir Karimi):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:20>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:21>
Django
-----------------------------+---------------------------------------
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:22>
Django
-----------------------------+---------------------------------------
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:23>
Django
-----------------------------+---------------------------------------
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:24>
Django
-----------------------------+---------------------------------------
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:25>
Django
Reporter: Tom Forbes | Owner: Amir Karimi
Type: New feature | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:26>
Django
-----------------------------+---------------------------------------------
Reporter: Tom Forbes | Owner: Amir Karimi
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"27a3eee72170f1eb994a213db985f42c6cf5f994" 27a3eee7]:
{{{
#!CommitTicketReference repository=""
revision="27a3eee72170f1eb994a213db985f42c6cf5f994"
Fixed #31700 -- Made makemigrations command display meaningful symbols for
each operation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31700#comment:27>