mfogel / django-timezone-field

A Django app providing DB, form, and REST framework fields for zoneinfo and pytz timezone objects.
BSD 2-Clause "Simplified" License
397 stars 95 forks source link

Change base class of TimeZoneSerializerField from DJRF's Field to CharField #137

Closed mfogel closed 4 months ago

mfogel commented 4 months ago

What

Why

Subclassing Field introduces some situations where it isn't clear what the correct behavior is. DJRF addressed this for CharField in their 3.0 release - let's leverage their solution here.

Closes #130

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.64%. Comparing base (bb20f86) to head (610a2c2).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #137 +/- ## ========================================== - Coverage 99.09% 98.64% -0.46% ========================================== Files 10 10 Lines 222 222 ========================================== - Hits 220 219 -1 - Misses 2 3 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gotounix commented 3 months ago

@mfogel I don't think changing Field to CharField is a good solution. And this change is a BREAKING change. Because in TimeZoneSerializerField 's __init__ using super().__init__(*args, **kwargs), but in CharField 's __init__ it does't support *arg:

class CharField(Field):
    ...

    def __init__(self, **kwargs):

Therefore, changing Field to CharField not only loses flexibility, but also makes all existing subclasses of TimeZoneSerializerField that using *args unusable. I believe a better solution is to use a custom validator instead of changing the original default behavior to fit the DRF's parameters.