Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
fix: MultipleChoiceField use ordered sort#9735
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please also add unit tests to verify this and make the CI green
fbozhang commentedJul 5, 2025
I’ve added the tests and the CI is green now. |
Uh oh!
There was an error while loading.Please reload this page.
browniebroke left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems sensible to me 👍🏻
However, it will have to wait for 3.17 as I see this as potentially disruptive for some downstream users. Left a small suggestion
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| assertserializer.is_valid() | ||
| assertserializer.validated_data['nested']['example']=={1,2} | ||
| assertserializer.validated_data['nested']['example']==[1,2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hum, that's a bit unfortunate that this data type is exposed at this level. I'm thinking of potential users testing at the serializer level might get some new test failures with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we just need to note in the new version's documentation that the return type is a list, so that if a user upgrades and their tests fail, they'll know exactly what to look at.
Since the input, as mentioned in the signature, is already an ordered sequence (a list or a tuple), it would be much more user-friendly if the output keeps that same order.
fbozhang commentedJul 6, 2025
Good point — waiting for 3.17 sounds like the right call. Appreciate the suggestion! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
This PR modifiesMultipleChoiceField to return ordered lists instead of sets, ensuring JSON serialization compatibility and maintaining the order of input values. The change usesdict.fromkeys() to deduplicate values while preserving insertion order.
Key Changes
- Changed
to_internal_value()andto_representation()methods to return lists usinglist(dict.fromkeys([...])) - Updated all test expectations from sets to lists
- Added JSON serialization tests to prevent regression
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rest_framework/fields.py | ModifiedMultipleChoiceField methods to return ordered lists instead of sets usingdict.fromkeys() for deduplication |
| tests/test_fields.py | Updated test expectations from sets to lists, added order-preserving test case, and added JSON serialization validation tests |
| tests/test_serializer_nested.py | Updated nested serializer test assertions to expect lists instead of sets |
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
| valid_inputs= { | ||
| ():set(), | ||
| ('aircon',): {'aircon'}, | ||
| ('aircon','manual'): {'aircon','manual'}, | ||
| ():list(), | ||
| ('aircon',): ['aircon'], | ||
| ('aircon','manual'): ['aircon','manual'], | ||
| ('manual','aircon'): ['manual','aircon'], | ||
| } |
CopilotAIDec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missing test case for duplicate values. The change to usedict.fromkeys() deduplicates values, but there's no test case to verify this behavior. Consider adding a test case like('aircon', 'aircon'): ['aircon'] to ensure duplicates are properly handled and deduplicated while maintaining order.
| outputs= [ | ||
| (['aircon','manual','incorrect'], {'aircon','manual','incorrect'}) | ||
| (['aircon','manual','incorrect'], ['aircon','manual','incorrect']), | ||
| (['manual','aircon','incorrect'], ['manual','aircon','incorrect']), | ||
| ] |
CopilotAIDec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missing test case for duplicate values in the outputs. Theto_representation method now deduplicates values while maintaining order, but there's no test to verify this behavior. Consider adding a test case with duplicate values like(['aircon', 'manual', 'aircon'], ['aircon', 'manual']) to ensure the deduplication logic works correctly.
This modification has the following advantages: