Adds deconstruct methods to the US flavor.

[mlissner]

Format is cribbed from the deconstruct method's documentation, which provides clear examples for how to write these methods. To do this, I needed to import the models into the tests, which created some fun namespace problems.

[mlissner]

Forgot to mention, this fixes #161.

[jezdez]

Hey @mlissner, this seems to make sense. But wouldn't using the django.utils.deconstruct.deconstructible decorator be enough to capture the parameters and build a deconstruct method? As to the other non-US localflavors, I'd appreciate if you could work on those as well to have a consistent code base. Thanks!

[mlissner]

I'm not sure if the decorator would be enough. From the example given, which I copied for the changes I used here, it seems like the decorator wouldn't be enough, but I'm really not sure how to figure out when it's essential to have a deconstruct method and when the decorator will suffice. I've been wanting to find time to look into that and do some experimentation, but I haven't been able to find time. (Indeed, here I am working on the weekend already). If you or somebody else can provide me with that direction, I can do the slog of applying the decorator (or the full method) everywhere in the codebase.

[claudep]

Shouldn't this be >=? In practice, this shouldn't change anything as all 1.7 releases will probably be greater than (1, 7), but semantically, I think it would be clearer.

[mlissner]

Yeah, good call. If we can figure out how to do this properly, I can include that change.

[claudep]

What do you mean by "do this properly"?

[mlissner]

My previous question about decorator vs full method.

On Mon, Aug 10, 2015, 07:26 Claude Paroz notifications@github.com wrote:

What do you mean by "do this properly"?

—
Reply to this email directly or view it on GitHub
#162 (comment)
.

[claudep]

Theoretically, the tests could be used to check if the decorator is adequate or not.

[claudep]

I just tried to run your tests without the deconstruct addition (on Django 1.8), and the tests still run fine. Could you maybe tell us the error you encounter in your project?

[mlissner]

I saw this as a potential problem as I was upgrading my project to Django 1.7 and reading the release notes. In there, it says:

This change should not affect you unless you write custom Field subclasses; if you do, you may need to reimplement the deconstruct() method if your subclass changes the method signature of init in any way. If your field just inherits from a built-in Django field and doesn’t override init, no changes are necessary.

At that point I realized that I used localflavor's custom fields, and I checked if there were deconstruct methods and whether the __init__ signature was changed. Finding that no, there were no deconstruct methods and that yes, the signature was changed, I got to work.

I'm no expert on what this method does or why it's important, but the docs say it is, so I expect problems during migrations if it's absent.

[benkonrath]

@mlissner We just dropped support for Django 1.5 & 1.6 (#170) and we'll probably also drop 1.7 support shortly now that it's unsupported. You can remove the conditional in the tests.

As for your outstanding question, the migration documentation says that @deconstructible is only for custom class instances, not model fields.
https://docs.djangoproject.com/en/1.9/topics/migrations/#adding-a-deconstruct-method

It looks like we need the deconstruct() method on any model field that adds extra options to __init__()

If you haven’t added any extra options on top of the field you inherited from, then there’s no need
to write a new deconstruct() method. If, however, you’re changing the arguments passed in
init(), you’ll need to supplement the values being passed.

https://docs.djangoproject.com/en/1.9/topics/migrations/#adding-a-deconstruct-method

It would be great if you could implement deconstruct() for all model fields in localflavor that need them. Thanks a lot for pointing out this issue and helping to solve it.

Edit: Change incorrect references about destruct to deconstruct.

[benkonrath]

@mlissner Just a quick update on this. The US model fields don't need the deconstruct() method because they inherit from CharField. Your test passes without the deconstruct() method as @claudep pointed out. I still like the idea of having the tests on the US model fields. just to verify that it's working as we expect.

The task now is to add deconstruct() method to the model fields that add extra options to __init__(). Offhand I know the IBAN and BIC fields do this but there could be other model fields that need this as well.

[benkonrath]

As I understand things, the US models don't need the deconstruct method so I'm closing this PR. I'm keeping #161 open as a reminder that this needs to fixed in the other models.

[benkonrath]

After looking that this again, I think the changes in this PR are correct. While it's not strictly required to include the deconstruct method for the case of the US fields, the Django documentation does recommend it.

I'm planning to build on this PR to add deconstruct to the other model fields that need them. I've reopened this PR as a reminder for this work. Sorry about the confusion.