django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #27704 -- Used TypedMultipleChoiceField for ArrayFields with choices in base_field.

Open Anv3sh opened this issue 1 year ago • 2 comments

As per suggestions in @ngnpope's comment on #7850 made choices_form_class = TypedMultipleChoiceField also removed support for ArrayField.choices and instead ArrayField.base_field.choices shall be used. Removed ArrayField.clean() method intoduced in https://github.com/django/django/commit/9dd244394236388c3479ab202a0ec31055f7ec09 because SimpleArrayField will no more be used while ArrayField has choices and also shifted the test test_model_field_choices introduced in https://github.com/django/django/commit/9dd244394236388c3479ab202a0ec31055f7ec09 to tests.postgres_tests.test_array.TestChoiceFormField.

Anv3sh avatar Jun 28 '22 19:06 Anv3sh

@ngnpope Do you want to take a look?

felixxm avatar Jul 12 '22 07:07 felixxm

Committed the changes according to @ngnpope's review and added test for it although we still need to look for a fix for some shortcomings that I picked out in this comment.

Anv3sh avatar Jul 31 '22 14:07 Anv3sh

@felixxm if someone could review 🙇🏻

Anv3sh avatar Sep 21 '22 21:09 Anv3sh

@ngnpope can you have a look??

Anv3sh avatar Nov 10 '22 06:11 Anv3sh

@jacobtylerwalls committed the suggested changes although the patch does not throws a warning if choices are not in base field as far as i tested even though we have that in the patch and also the test works fine.

Anv3sh avatar Jan 01 '23 21:01 Anv3sh

Ah, along the lines of this https://github.com/django/django/pull/15805#discussion_r921342795?

You could consider changing the warning to a system check as Nick suggested.

jacobtylerwalls avatar Jan 01 '23 22:01 jacobtylerwalls

Ah, along the lines of this #15805 (comment)?

You could consider changing the warning to a system check as Nick suggested.

Got it 👍🏻 will look into it

Anv3sh avatar Jan 03 '23 07:01 Anv3sh

Ah, along the lines of this #15805 (comment)?

You could consider changing the warning to a system check as Nick suggested.

So we would have to raise an error on running the makemigrations command right? Also according to our current change that wouldn't be possible cause we would only be able to check it once the ArrayField.formfield() is called.

Anv3sh avatar Feb 02 '23 04:02 Anv3sh

Have a look at the documentation for field checks. This is what I was referring to.

ngnpope avatar Feb 02 '23 09:02 ngnpope

Closing due to inactivity.

felixxm avatar Jul 25 '23 04:07 felixxm