Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deconstruct methods to fields with custom keywords #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

roadsideseb
Copy link

Fields that are adding keyword arguments to custom fields require a deconstruct method to make the keywords available in the corresponding migration(s). The Django 1.7 documentation provides more details.

Without the deconstruct method, the custom keywords are not added to the migrations and are therefore ignored when the migrations are run. In my case, the migrations failed because auto_add was not set during the migrations of the UUIDField.

I have added deconstruct methods to th ArrayField, JSONField and UUIDField adding the custom keywords accordingly. Please take a look and let me know if you are happy with that or have any suggestions to improving it.

Oh, and thanks for a great Django app 👍.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 37915b9 on elbaschid:fix/add_deconstruct_methods_for_django_17 into 4930634 on lukesneeringer:master.

@lukesneeringer
Copy link
Owner

Hi @elbaschid:
Thanks for doing this! I'll look it over as soon as is feasible with the intent of getting it merged in. I'll comment if I have any issues.

I've been wanting to deep-dive into what I need to do in order to support Django 1.7, but you've beaten me to it, which I greatly appreciate. :)

@roadsideseb
Copy link
Author

Great, thanks.

@lukesneeringer
Copy link
Owner

Hey @elbaschid: This looks great, but lacks tests.

Also, while it shouldn't block getting this in, obviously CompositeField probably needs to get a deconstruct method also.

Edit: I've just created #14 for a CompositeField destruct, which I can do later. The only thing blocking this PR is getting tests.

@lukesneeringer
Copy link
Owner

Also, add yourself to AUTHORS! :)

@roadsideseb
Copy link
Author

Thanks for the feedback. You are absolutely right, that there need to be tests. I'll put those in. Would you be happy with testing the deconstruct methods in isolation or do you want something more comprehensive?

Fields that are adding keyword arguments to custom fields require
a ``deconstruct`` method to make the keywords available in the
corresponding migration(s). The documentation for this can be found
here:

https://docs.djangoproject.com/en/dev/howto/custom-model-fields/#field-deconstruction
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1fae990 on elbaschid:fix/add_deconstruct_methods_for_django_17 into e053f14 on lukesneeringer:master.

@roadsideseb
Copy link
Author

I've started adding some test for the deconstructmethods in isolation. I've started looking at extending the south_migrations tests to work with the new migrations but I am having a bit of trouble with your redirect_std raising an exception using it with call_command('makemigrations', 'south_migrations') (stacktrace below). Any suggestions why that would be an issue? I'll take a closer look and will try and put together some tests and update the PR.

Traceback (most recent call last):
  File "/home/elbaschid/Worx/Roadside/django-pgfields/tests/south_migrations/tests.py", line 155, in setUp
    dry_run=True, verbosity=3)
  File "/home/elbaschid/.virtualenvs/pgfields/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 115, in call_command
    return klass.execute(*args, **defaults)
  File "/home/elbaschid/.virtualenvs/pgfields/local/lib/python2.7/site-packages/django/core/management/base.py", line 336, in execute
    self.check()
  File "/home/elbaschid/.virtualenvs/pgfields/local/lib/python2.7/site-packages/django/core/management/base.py", line 415, in check
    self.stderr.write(msg)
  File "/home/elbaschid/.virtualenvs/pgfields/local/lib/python2.7/site-packages/django/core/management/base.py", line 73, in write
    self._out.write(force_str(style_func(msg)))
TypeError: unicode argument expected, got 'str'

I've also changed the implementation of the deconstruct methods slightly. I'm not checking the values against the default values. This means that keyword arguments will always be written to the migration files. The reason for this is, that this will result in consistent behaviour when running migrations even if a default value has to be changed at some point in the future. Not sure if you'd agee with me on that. Let me know your thoughts and I'll be happy to revert back to the original implementation.

@lukesneeringer
Copy link
Owner

Would you be happy with testing the deconstruct methods in isolation or do you want something more comprehensive?

Isolation is fine IMO.

@lukesneeringer
Copy link
Owner

I've started looking at extending the south_migrations tests to work with the new migrations but I am having a bit of trouble with your redirect_std raising an exception using it with call_command('makemigrations', 'south_migrations') (stacktrace below).

This looks like a typecasting problem. io.StringIO is expecting to get a unicode object and is getting a bytestring instead.

A few questions:

  • What version of Python is this? If it's 2, what happens under 3, and vice versa? Ultimately it needs to work under both, but that will at least tell us something. My vague recollection of Django's force_str is that it makes a literal str object regardless of the Python version, which could be annoying, since that's a bytestring under Python 2 and a text string under Python 3. If that's the case, we might need to change that io object to something more permissive. I'm not sure if there's a version of StringIO or BytesIO that will just take either one.
  • I don't have much context for this test. Are you trying to add a new test and use the same construct and it's not working, or did you do something that caused my existing tests to fail? (It's fine if it's the latter; I'm just trying to get a better grasp of the problem).

I've also changed the implementation of the deconstruct methods slightly. I'm not checking the values against the default values. This means that keyword arguments will always be written to the migration files. The reason for this is, that this will result in consistent behaviour when running migrations even if a default value has to be changed at some point in the future. Not sure if you'd agee with me on that. Let me know your thoughts and I'll be happy to revert back to the original implementation.

Original implementation. I want to follow how Django does things as much as possible, and they consider defaults to be sacrosanct once released into the wild.

@roadsideseb
Copy link
Author

Thanks for the feedback. I've been a bit busy over the last few days and will be for the rest of the week. I'll update the PR as soon as I get some time and get back with some further details on some of your comments.

@lukesneeringer
Copy link
Owner

Sounds good! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants