id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
30870	"""migrate --plan"" outputs ""IRREVERSIBLE"" on RunPython operations without docstrings."	Kyle Dickerson	Mariusz Felisiak	"Given a migration like:
{{{#!python
from django.db import migrations

def forward(apps, schema_editor):
    pass

def reverse(apps, schema_editor):
    pass

class Migration(migrations.Migration):
    operations = [
        migrations.RunPython(forward, reverse)
    ]
}}}

`manage.py migrate --plan` will output:

{{{
Planned operations:
example.0001_initial
    Raw Python operation -> IRREVERSIBLE
}}}

The migration should not be described as ""irreversible"".

This error is in the definition of `describe_operation` in `django/django/core/management/commands/migrate.py`, reproduced below with line numbers from 2.2.6 tag.

{{{#!python
343    @staticmethod
344    def describe_operation(operation, backwards):
345        """"""Return a string that describes a migration operation for --plan.""""""
346        prefix = ''
347        if hasattr(operation, 'code'):
348            code = operation.reverse_code if backwards else operation.code
349            action = code.__doc__ if code else ''
350        elif hasattr(operation, 'sql'):
351            action = operation.reverse_sql if backwards else operation.sql
352        else:
353            action = ''
354            if backwards:
355                prefix = 'Undo '
356        if action is None:
357            action = 'IRREVERSIBLE'
358            is_error = True
359        else:
360            action = str(action).replace('\n', '')
361            is_error = False
362        if action:
363            action = ' -> ' + action
364        truncated = Truncator(action)
365    return prefix + operation.describe() + truncated.chars(40), is_error
}}}


Line 349 uses the docstring as the output string.
Line 356 tests that value and sets `action = 'IRREVERSIBLE'` on line 357 because the dosctring is `None`.

It would appear that the intention is to use a docstring to describe the operation, if available, and leave it blank otherwise.  However, because it tests against `code` instead of `code.__doc__` it actually sets `action = None` resulting in 'IRREVERSIBLE' being displayed.

== Proposed Solutions below

For a localized fix, I believe line 349 should be replaced by
{{{#!python
        if code:
            action = code.__doc__ if code.__doc__ else ''
        else:
            action = None
}}}

However, a more holistic view suggests that displaying ""IRREVERSIBLE"" isn't really the correct thing to do.  ""IRREVERSIBLE"" is set when `is_error` is also set to `True` and seems to be trying to indicate that the migration operation is invalid rather than irreversible.  That is, if `code`/`reverse_code` is None (line 348) or `sql`/`reverse_sql` is None (line 351) the migration can't run.

Since `sql` and `code` are required parameters for their respective Operations, `action` should only possibly be None in the reverse case, which seems to be what this code is trying to capture and explain.

Given that, a better approach would probably make use of the `reversible` property defined on `RunSQL` and `RunPython` operations.  This is a little verbose and could probably be pared down, but I think it has the right logic:

{{{#!python
@staticmethod
def describe_operation(operation, backwards):
    """"""Return a string that describes a migration operation for --plan.""""""
    prefix = ''
    action = ''
    is_error = False

    if backwards:
        prefix = 'Undo '
        if hasattr(operation, 'reversible') and not operation.reversible:
            action = 'INVALID'
            is_error = True
        elif hasattr(operation, 'reverse_code'):
            action = operation.reverse_code.__doc__ if operation.reverse_code.__doc__ else ''
        elif hasattr(operation, 'reverse_sql'):
            action = operation.reverse_sql.__doc__ if operation.reverse_sql.__doc__ else ''
    else:
        if hasattr(operation, 'code'):
            action = operation.code.__doc__ if operation.code.__doc__ else ''
        elif hasattr(operation, 'sql'):
            action = operation.sql.__doc__ if operation.sql.__doc__ else ''

    action = ' -> ' + str(action).replace('\n', '')
    truncated = Truncator(action)
    return prefix + operation.describe() + truncated.chars(40), is_error
}}}"	Bug	closed	Core (Management commands)	2.2	Release blocker	fixed	migrate		Accepted	1	0	0	0	0	0