Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. #10997

tim-schilling · 2019-02-15T15:10:47Z

When verbosity is 2 or greater and listing migrations with
showmigrations, include the applied datetime in the output.

charettes

Any thoughts about changing plan nodes to be three elements (app, name, applied=None) instead of using this custom tuple subclass and loosing the containment check abilities on the returned set?

charettes · 2019-02-15T15:17:51Z

django/db/migrations/recorder.py

-            return {tuple(x) for x in self.migration_qs.values_list('app', 'name')}
+            return {
+                AppliedMigration([m.app, m.name], applied=m.applied)
+                for m in self.migration_qs.iterator()


It don't think it's worth using iterator() here given the small size of the result set and the fact it will be fully materialized. Also you could keep using values_list('app', 'name', 'applied').

class AppliedMigration(tuple): def __init__(self, app, name, applied): instance = tuple.__new__(cls, (app, name)) instance.applied = applied return instance return {AppliedMigration(x) for x in self.migration_qs.values_list('app', 'name', 'applied')

Sounds good.

charettes · 2019-02-15T15:25:20Z

django/core/management/commands/showmigrations.py

@@ -84,9 +86,15 @@ def show_list(self, connection, app_names=None):
                        title = plan_node[1]
                        if graph.nodes[plan_node].replaces:
                            title += " (%s squashed migrations)" % len(graph.nodes[plan_node].replaces)
+                        # Get the applied migration instance
+                        filtered = filter(lambda m: m == plan_node, loader.applied_migrations)


This was a problem before as well but I guess it should be fixed while you are around.

In the case where showmigrations is not called with an app label (or called with multiple ones) this will incur len(app_names) queries since each access to loader.applied_migrations incurs one. It might be worth assigning a applied_migrations = loader.applied_migrations variable outside of the loop.

Wait, loader.applied_migrations is not the same as Recorder.applied_migrations() where the latter actually makes a DB query. loader.applied_migrations is a property that doesn't make any DB queries, but gets set in build_graph()

Ahh never mind then, I assumed it was the same function based on the name.

tim-schilling · 2019-02-15T15:37:24Z

Any thoughts about changing plan nodes to be three elements (app, name, applied=None) instead of using this custom tuple subclass and loosing the containment check abilities on the returned set?

@timgraham Brought up a similar issue. I implemented this way because there are a number of places where there are comparisons to the results of applied_migrations. Theses comparisons are done where the other element is from some other location that wouldn't have an applied datetime. For example, when checking to see if a migration was applied it, the disk migration would have (app, name, None) but applied_migrations() would return (app, name, datetime). The applied datetime is just additional information, but this would cause these checks to fail in their current state requiring me to re-write them.

There are three proposed options so far, this tuple subclass I have, Tim suggested returning the Migration model instances or changing this to a three element tuple as you suggested. A fourth option would be to return a dict where the key is the (app, name) tuple and the value is the Migration object. That would make the comparisons easier with disk migrations and avoid a weird subclass.

charettes · 2019-02-15T15:40:58Z

Thanks for the summary @tim-schilling!

A fourth option would be to return a dict where the key is the (app, name) tuple and the value is the Migration object. That would make the comparisons easier with disk migrations and avoid a weird subclass.

I like this option a lot. It preserves the current the set'ish nature of applied_migration while exposing the actual migration object for .applied access.

carltongibson

Hi @tim-schilling.

This looks super. Thanks.

I just left a few minor comments. I'm not insistent on those...

Can you rebase and squash to a single commit?

I'm going to mark this RFC. Good stuff!

carltongibson · 2019-03-07T10:00:49Z

django/db/migrations/loader.py

-                            migration[0], migration[1], parent[0], parent[1],
-                            connection.alias,
+                            migration_key[0], migration_key[1], parent[0],
+                            parent[1], connection.alias,


This could go on one line I think.

(Would be less than 120 chars and splitting parent[0] from parent[1] is a little weird.)

Ah, I thought the limit was 80 characters. I was wondering why there were so many offending lines.

carltongibson · 2019-03-07T10:04:39Z

django/db/migrations/recorder.py

-            return {tuple(x) for x in self.migration_qs.values_list('app', 'name')}
+            return {
+                (migration.app, migration.name): migration
+                for migration in self.migration_qs


Single line?

carltongibson · 2019-03-07T10:06:02Z

django/db/migrations/recorder.py

@@ -69,13 +69,16 @@ def ensure_schema(self):
            raise MigrationSchemaMissing("Unable to create the django_migrations table (%s)" % exc)

    def applied_migrations(self):
-        """Return a set of (app, name) of applied migrations."""
+        """Return a dict of (app, name), Migration of applied migrations."""


Maybe "Return a dict of (app, name) to Migration instances of applied migrations."

(Just not sure the comma reads...)

Works for me.

carltongibson · 2019-03-07T10:12:50Z

docs/releases/3.0.txt

@@ -259,7 +262,7 @@ to remove usage of these features.
 * Support for the ``context`` argument of ``Field.from_db_value()`` and
  ``Expression.convert_value()`` is removed.

-* The ``field_name`` keyword argument of ``QuerySet.earliest()` and
+* The ``field_name`` keyword argument of ``QuerySet.earliest()`` and


Merged this as 38c3f7e. Thanks!

tim-schilling · 2019-03-07T13:55:08Z

Implemented your suggestions, squashed and rebased. Thanks for the review!

timgraham · 2019-03-07T15:59:52Z

Could you send the refactoring code/test changes as a separate PR so that's not tied up with the feature change? Then we can rebase this and it'll look less invasive.

tim-schilling · 2019-03-07T16:09:28Z

@timgraham I'm confused. I should have two PRs? One with the changes to django (with failing tests) and then the other with the tests for non-existing functionality that fail?

Or would the second include the functionality changes?

timgraham · 2019-03-07T16:34:41Z

What I meant as "the refactoring" is all the changes for changing the return type of applied_migrations().

tim-schilling · 2019-03-07T16:37:31Z

That was refactoring in order to make the feature possible. I can't separate them.

timgraham · 2019-03-07T16:39:48Z

The refactoring can precede the feature, no?

tim-schilling · 2019-03-07T16:44:51Z

Yes. Sorry, I (for some reason) assumed the order was the opposite. If that's what works best, I can break it up that way. What do I title the refactoring PR?

timgraham · 2019-03-07T16:47:01Z

Refs #30186 -- Changed MigrationRecorder.applied_migrations() to return a dict.

tim-schilling · 2019-03-07T16:48:45Z

Thanks for the help.

tim-schilling · 2019-03-07T16:58:45Z

Did you want me to rebase this off of master without the commit from #11058?

…mes at verbosity 2+.

tim-schilling force-pushed the ticket_30186 branch from 99ab5be to 436b63b Compare February 15, 2019 15:25

charettes reviewed Feb 15, 2019

View reviewed changes

tim-schilling force-pushed the ticket_30186 branch from 566c6c0 to c8ec2ea Compare February 15, 2019 16:53

timgraham changed the title ~~Show the applied datetime in verbose showmigrations.~~ Fixed #30186 -- Allowed showmigrations --list to display the applied datetimes. Feb 20, 2019

carltongibson reviewed Mar 7, 2019

View reviewed changes

tim-schilling force-pushed the ticket_30186 branch from c8ec2ea to e46aa5e Compare March 7, 2019 13:53

tim-schilling force-pushed the ticket_30186 branch from e46aa5e to f2369ec Compare March 7, 2019 16:57

timgraham changed the title ~~Fixed #30186 -- Allowed showmigrations --list to display the applied datetimes.~~ Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. Mar 8, 2019

timgraham force-pushed the ticket_30186 branch 2 times, most recently from ef4f352 to c78b848 Compare March 8, 2019 01:04

Fixed #30186 -- Made showmigrations --list display the applied dateti…

7c68cea

…mes at verbosity 2+.

timgraham force-pushed the ticket_30186 branch from c78b848 to 7c68cea Compare March 8, 2019 13:25

timgraham merged commit 7c68cea into django:master Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. #10997

Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. #10997

tim-schilling commented Feb 15, 2019

charettes left a comment

charettes Feb 15, 2019

tim-schilling Feb 15, 2019

charettes Feb 15, 2019

tim-schilling Feb 15, 2019

tim-schilling Feb 15, 2019

charettes Feb 15, 2019

tim-schilling commented Feb 15, 2019

charettes commented Feb 15, 2019

carltongibson left a comment

carltongibson Mar 7, 2019

tim-schilling Mar 7, 2019

carltongibson Mar 7, 2019

carltongibson Mar 7, 2019 •

edited

tim-schilling Mar 7, 2019

carltongibson Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. #10997

Fixed #30186 -- Made showmigrations --list display the applied datetimes at verbosity 2+. #10997

Conversation

tim-schilling commented Feb 15, 2019

charettes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-schilling commented Feb 15, 2019

charettes commented Feb 15, 2019

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson Mar 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

timgraham commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

tim-schilling commented Mar 7, 2019

carltongibson Mar 7, 2019 •

edited