Fixed #26151 -- Refactored MigrationWriter.serialize()

[yoongkang]

The serializer() method had a McCabe score of 50, one
of the highest in the entire Django code base.

The serialization logic was moved into different Serialization
classes, and a Serializer factory returns the right
Serializer based on the value type.

[MarkusH]

What do you think about dropping the __init__() and making serialize() a classmethod that takes the value it serializes. That way you can avoid the instantiation of serializers.

[MarkusH]

Essentially

class BaseSerializer(object):
    @classmethod
    def serialize(self, value):
        # do something with value

[yoongkang]

Yeah, I don't know. I didn't do that mainly because I had to this:

class serialize(cls, value):
    serializer_factory(value).serialize(value)

The individual serializers sometimes have to serialize their components (for example iterables), which meant I'd have to pass in the value twice each time.

Also, it's possible we might want to add methods to the serializers which depend on the value, though that didn't end up happening very often (except TupleSerializer).

[MarkusH]

Is there any reason why you moved this into a separate method?

[yoongkang]

I actually moved it from MigrationWriter. I guess it doesn't need to be a separate method. I'll amend this shortly.

[MarkusH]

I'd probably reorder the serializer classes alphabetically where possible and move all BaseFooSerializer classes to the top.

[MarkusH]

Doesn't this one do the exact same thing as the TupleSerializer?

[yoongkang]

It's very similar, but it uses len(strings) instead of len(self.value). We could have a property or method called length_to_serialize() that returns strings for Iterables or self.value for Tuples, but I wasn't sure it was worthwhile.

Do you think I should add that in IterableSerializer and have TupleSerializer override it?

[MarkusH]

Gotcha! When you replace the IterableSerializer with the TupleSerializer in the factory you end up with this failing test. I think we can leave it as as.

Traceback (most recent call last):
  File "/home/markus/Coding/django/tests/migrations/test_writer.py", line 362, in test_serialize_settings
    ("((0, 0), (1, 1), (2, 4))", set())
  File "/home/markus/Coding/django/tests/migrations/test_writer.py", line 187, in assertSerializedResultEqual
    self.assertEqual(MigrationWriter.serialize(value), target)
  File "/home/markus/Coding/django/django/db/migrations/writer.py", line 308, in serialize
    return serializer_factory(value).serialize()
  File "/home/markus/Coding/django/django/db/migrations/serializer.py", line 46, in serialize
    format = self._format()
  File "/home/markus/Coding/django/django/db/migrations/serializer.py", line 303, in _format
    return "(%s)" if len(self.value) != 1 else "(%s,)"
TypeError: object of type 'generator' has no len()

[MarkusH]

@timgraham do you recon this change is worth mentioning in the 1.10 release notes?

[MarkusH]

There's a leftover serialize_datetime() in the MigrationWriter that seems unused to me.

[timgraham]

Does it add any features or make any backwards incompatible changes? If not, I'd say no.

[timgraham]

merged in 4b1529e, thanks!