search

django / django-localflavor

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

Inherit from CharField instead of Field #446

Closed benkonrath closed 2 years ago

benkonrath commented 3 years ago

A first (incomplete) PR for converting the fields inheriting from Field to inheriting from CharField.

Fixes #445

claudep commented 3 years ago

I would also make a separate PR for isort changes.

benkonrath commented 3 years ago

The isort changes are mostly relevant but I get your point - reverted.

benkonrath commented 2 years ago

@claudep This is ready for review again - no rush, whenever you have a chance.

I think it's better to just include these changes in the 4.0 release. These fields have been setup to inherit from Field since they were created - that goes as far back as django-localflavor 1.0 and probably even into the original code in Django itself. I don't think 3.1.1 release makes sense for this change.

claudep commented 2 years ago

Thanks Ben, looks rather good. I don't remember, but are those strip parts really needed in this patch? Once again, I would have preferred that to be in a separate PR (with a possible factorization), but they are maybe required by tests?

benkonrath commented 2 years ago

Fields that require strip to be True in some way rely on the value being run through strip(). We could keep that code as is and continue to call strip() in the clean() method of the field itself. However, that would mean the field would have the option to set strip=False with no effect. As I user I would expect to get some sort of notice if a documented option isn't working.

For the next steps, we probably could re-write the code to allow the strip option to function but I thought that keeping it to smallest change would be better (hence this solution). In the end, most people use the default strip=True. If somebody needs stripe=False for one of these fields, we can fix it then or ask them for help. Maybe adding an issue about this to remind us... though it would be low on my personal priority list just because stripe=False is probably not used too much as I mentioned.

I'm happy to adjust things if you don't agree with my reasoning.

Merging #461 first and then rebasing this PR would make it a little smaller. If you have a chance, it would be nice if you could double check that one as well.

benkonrath commented 2 years ago

Hmm, #461 had less overlap than I thought it would have. In any case, I think all of the changes in this PR are related to changing to CharField and taking advantage of the API it provides. @claudep If you have no objections, I'd like to merge this as is.

claudep commented 2 years ago

The more I think about it, the less I like those strip parts. As you mentions, the default is True anyway, so I don't see the point in twiddling with that. I tried your PR without those parts in #462 and tests seem to pass (at least locally).

benkonrath commented 2 years ago

That's fine. Feel free to merge your modified version.