django / django-localflavor

Country-specific Django helpers, formerly of contrib fame
https://django-localflavor.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
828 stars 290 forks source link

Field of CharField inheritance #445

Closed claudep closed 2 years ago

claudep commented 3 years ago

Most of localflavor form fields inherit from CharField. However, some are directly inheriting from Field. Whenever you use those fields as the default form field for any model CharField, you'll get the __init__() got an unexpected keyword argument 'max_length' TypeError. I wonder if the fact some fields inheriting from Field only is a desing decision or simply an oversight.

@benkonrath, what do you think?

benkonrath commented 3 years ago

I suspect this is an oversight. I vaguely recall noticing this in the past but assumed that these fields were intentionally setup to inherit directly from Field. Since there's a specific issue with this setup, I think we should change all text based fields to inherit from CharField.

I think it's worthwhile to make a 3.1.1 release to address this. Do you see any compatibility issues with this change?

benkonrath commented 3 years ago

I can make a PR later this afternoon if you haven't already beat me to it.

claudep commented 3 years ago

Such changes could always give backwards incompatibilities, however, I think it's the right thing to do, and we can document the affected fields in the ChangeLog. I'll be pleased to review your patch :-)

benkonrath commented 3 years ago

There were more than I thought there would be and I have to do other stuff now. I'll finish the PR in the next couple of days and let you know when it's ready for review.

claudep commented 3 years ago

Sure, no rush! Just tell me if you'd like some help to cook the patches.

benkonrath commented 3 years ago

I can do it - it's giving me more motivation to finish my work on a Django wrapper to python-stdnum and possibly/eventually deprecate most of this stuff. :-)

claudep commented 2 years ago

Fixed by d7ba69b477