Opened 4 years ago

Closed 4 years ago

#31769 closed Cleanup/optimization (fixed)

Improve default name of merge migrations.

Reported by: Jon Dufresne Owned by: Jon Dufresne
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: 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

Currently, merge migrations filenames are created with a timestamp. For example:

0003_merge_20160102_0304.py

This name is more opaque than necessary. When one reads it, it isn't immediately clear which migrations were merged. One must inspect the file to find that information.

Instead, I suggest the default filename should be created by combining the files being merged. This way, it includes the information without requiring one to inspect the file. This information is also useful in the migrate command's output. As it is most likely to merge two migrations files, this should remain a reasonable length.

For example:

0003_merge_0002_conflicting_second_0002_second.py

If preferable, we could decide to join the names in other ways too. For example, separate names by __ (two underscores) or exclude prefix numbers. To start, I'll go with the easiest approach unless there is strong preference for a different naming scheme.

Change History (7)

comment:2 by Mariusz Felisiak, 4 years ago

Resolution: wontfix
Status: newclosed
Summary: Improve default name of merge migrationsImprove default name of merge migrations.

I'm not sure about this change because you may need to merge many migrations. In such case it seems that the timestamp is the best we can do, e.g. (with your patch):

$ python manage.py migrate

CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0002_mymodel1_field_1, 0002_mymodel10_mymodel2_mymodel3_mymodel4_mymodel5_mymodel6_mymodel7_mymodel8_mymodel9, 0002_mymodel101_mymodel21_mymodel31_mymodel41_mymodel51_mymodel61_mymodel71_mymodel81_mymodel91 in test_one).
To fix them run 'python manage.py makemigrations --merge'

$ python manage.py makemigrations --merge

Merging test_one
  Branch 0002_mymodel10_mymodel2_mymodel3_mymodel4_mymodel5_mymodel6_mymodel7_mymodel8_mymodel9
    - Create model MyModel1
    - Create model ...
  Branch 0002_mymodel1_field_1
    - Add field field_1 to mymodel1
  Branch 0002_mymodel101_mymodel21_mymodel31_mymodel41_mymodel51_mymodel61_mymodel71_mymodel81_mymodel91
    - Create model MyModel101
    - Create model ...

Merging will only work if the operations printed above do not conflict
with each other (working on different fields or models)
Do you want to merge these migration branches? [y/N] y

Traceback (most recent call last):
    ....
    res = handle_func(*args, **kwargs)
  File "/django/django/core/management/commands/makemigrations.py", line 140, in handle
    return self.handle_merge(loader, conflicts)
  File "/django/django/core/management/commands/makemigrations.py", line 309, in handle_merge
    with open(writer.path, "w", encoding='utf-8') as fh:
OSError: [Errno 36] File name too long: '/ticket_31769/test_one/migrations/0003_merge_0002_mymodel101_mymodel21_mymodel31_mymodel41_mymodel51_mymodel61_mymodel71_mymodel81_mymodel91_0002_mymodel10_mymodel2_mymodel3_mymodel4_mymodel5_mymodel6_mymodel7_mymodel8_mymodel9_0002_mymodel1_field_1.py'

Truncating merged names will be even more confusing.

You always use the --name option if you don't like a generated name (maybe we could ask for it in the interactive mode 🤔).

comment:3 by Jon Dufresne, 4 years ago

Resolution: wontfix
Status: closednew

I'm not sure about this change because you may need to merge many migrations. In such case it seems that the timestamp is the best we can do

In the case or merging lots of files, I agree the timestamp is the best we can do.

To solve this, we can add a threshold for the name length and if exceeded fallback to the old timestamp naming. This way, the common case of merging two or three migrations has a more useful name, but it doesn't get out of hand when more are involved. I'll update the PR with this change and include a test.

Having a threshold for auto-generated migration names has precedent, so we can carry that over:

https://github.com/django/django/blob/ae8338daf34fd746771e0678081999b656177bae/django/db/migrations/autodetector.py#L1272

The link uses a length of 100, which I'll use too. But I'm happy to tweak this larger or smaller to an agreeable size.

I'm also still open to dropping the migration number (0000_) if that makes the filename more readable. I personally like including the number, but I'm open to alternatives.

You always use the --name option if you don't like a generated name

Yes. This is the status quo and my current approach. But it doesn't solve my use case of providing more useful name by default. This is especially important when working on a diverse team where some members will not always remember to use useful names. Catching this before the code gets to team review is helpful for all by reducing repetitive feedback and by reducing the work a computer can do.

Beyond that, improving the default now means more projects will likely have more useful and reproducible filenames moving forward, regardless of their internal standards.

There have recently been a few other PRs, tickets and discussions on improving migration names. IMO, this ticket is along those same goals:

https://groups.google.com/forum/#!msg/django-developers/Bmcd779Wdl4/mlQRVNVgAQAJ
https://github.com/django/django/commit/6f3e3e87ab72eb64dcce8e646f8eea07be6c3b7e

comment:4 by Mariusz Felisiak, 4 years ago

Owner: changed from nobody to Jon Dufresne
Patch needs improvement: set
Status: newassigned
Triage Stage: UnreviewedAccepted

I'm also still open to dropping the migration number (0000_) if that makes the filename more readable. I personally like including the number, but I'm open to alternatives.

IMO numbers should be included.

comment:5 by Jon Dufresne, 4 years ago

Patch needs improvement: unset

comment:6 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 796be590:

Fixed #31769 -- Improved default naming of merged migrations.

47 gives 60 in total (47 + 5 + 5 + 3).

Note: See TracTickets for help on using tickets.
Back to Top