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

PRETTY_*_TIMEZONES_CHOICES is problematic with migrations #12

Open
PiDelport opened this issue Apr 3, 2018 · 9 comments
Open

PRETTY_*_TIMEZONES_CHOICES is problematic with migrations #12

PiDelport opened this issue Apr 3, 2018 · 9 comments

Comments

@PiDelport
Copy link
Collaborator

Currently, setting TimeZoneField's choices parameter to PRETTY_ALL_TIMEZONES_CHOICES or PRETTY_COMMON_TIMEZONES_CHOICES interacts pretty badly with Django migrations: every time any time zone's GMT offset changes, Django want to generate a new schema migration because of the differing choices list.

This is not very workable: we're facing a constant schema churn because of this, without any easy way around it (other than not using the PRETTY_*, but we want to display the offsets for user convenience).

Is there any reasonable way to avoid this problem somehow?

@michaeljohnbarr
Copy link
Owner

michaeljohnbarr commented Apr 3, 2018

Just so that make sure that I understand, any time an offset changes or any time a new timezone is added/removed, a new migration would be added to update the choices on the field.

The ask is that the change of an offset does not trigger a migration/schema change, but that the addition/removal of a timezone would trigger a schema migration (since the display of the offset is literally just a display value and not a change of the stored value in the database)?

@michaeljohnbarr
Copy link
Owner

michaeljohnbarr commented Apr 3, 2018

The only thing that I can think to do is to use the deconstruct() method on the custom fields to remove the choices from kwargs. Would this be acceptable?

def deconstruct(self):
    name, path, args, kwargs = super(TimeZoneField, self).deconstruct()
    kwargs.pop('choices', None)
    return name, path, args, kwargs

I don't have time right now to test this. However, if it works I definitely welcome a pull request.

There is already a deconstruct() method on the LinkedTZDateTimeField.

@PiDelport
Copy link
Collaborator Author

Hmm, this seems to be a slightly nuanced issue. There seems to be 3 possible directions:

  1. I've read upstream Django issues and discussions, and it seems choices gets tracked so data migrations have the right metadata to work with, and so on. However, having choices be omitted in deconstruct is an explicitly supported use case for fields that know the choices shouldn't affect the schema (which is arguably the case here?).

  2. There's also an open Django ticket to allow model field choices to be a callable (which would sidestep this problem, I presume?) but this is not implemented yet, so it's not a candidate solution yet.

  3. One possible alternative to futzing with the model field choices is to rework things to support having the choices just declared on the form fields? This would help indicate that they're more for display purposes only (especially with the pretty labels), rather than for actual data / schema purposes.

Do you have thoughts on the above?

@michaeljohnbarr
Copy link
Owner

@pjdelport

However, having choices be omitted in deconstruct is an explicitly supported use case for fields that know the choices shouldn't affect the schema (which is arguably the case here?).

I believe that your point above is the overall idea. Since choices is not currently callable, we cannot yet entertain this option (as you stated).

One possible alternative to futzing with the model field choices is to rework things to support having the choices just declared on the form fields? This would help indicate that they're more for display purposes only (especially with the pretty labels), rather than for actual data / schema purposes.

We definitely could add additional forms, but that's just more code to maintain down the line. My personal preference is the deconstruct() method, but I also am not necessarily biased between the two options. My thought is that the deconstruct() method exists which allows us to do what we are looking to do.

You are correct in saying that the forms would indicate they're more for display purposes. However, choices in general has two values; the left being the value to be stored, and the right being the display value, which is used in both the models and the forms. It's a tough call.

@PiDelport
Copy link
Collaborator Author

PiDelport commented Apr 12, 2018

It's a tough call.

Would it perhaps make sense to parametrise this with a field keyword argument that toggles the behaviour, so that users can choose whether they want choices to be included in migrations or not?

This way, the current behaviour can remain the default, and users like me who would rather have the other behaviour can set the flag in the next release that includes it.

@Surgo
Copy link
Collaborator

Surgo commented Sep 10, 2018

@pjdelport You can edit migration file directory.

import timezone_utils.fields
from timezone_utils.choices import PRETTY_COMMON_TIMEZONES_CHOICES  # Replace all choices when only initial create

class Migration(migrations.Migration):

    initial = True

    dependencies = [
        ('auth', '0008_alter_user_username_max_length'),
    ]

    operations = [
        migrations.CreateModel(
            name='User',
            fields=[
                ('timezone', timezone_utils.fields.TimeZoneField(choices=PRETTY_COMMON_TIMEZONES_CHOICES, default='UTC', max_length=32, verbose_name='timezone')),
                ...

@PiDelport
Copy link
Collaborator Author

@Surgo: Oh, that could work in the mean time, thanks!

@michaeljohnbarr
Copy link
Owner

https://code.djangoproject.com/ticket/24561 is the ticket we need to track for implementation. Not much movement yet, I'm afraid.

@lucasvazq
Copy link

🕸

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

No branches or pull requests

4 participants