Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #11929 -- Make sure YAML output order is helpful #11285

Closed
wants to merge 1 commit into from

Conversation

rixx
Copy link
Contributor

@rixx rixx commented Apr 26, 2019

By adopting Python 3.6+, this was already happening, but this test makes
sure we'll have no regression.

https://code.djangoproject.com/ticket/11929

@ngnpope
Copy link
Member

ngnpope commented Apr 29, 2019

I agree that ticket #11929 is resolved, but not purely by the fact that we are targeting Python 3.6+. I fixed this here by forcing PyYAML to treat dict as though it was an OrderedDict. This is because it used to force keys to be sorted. A "proper" fix would be to remove my workaround and add sort_keys=False here instead, but that would require enforcing pyyaml>=5.1.0 (which is probably not a bad thing given the security fix).

@rixx
Copy link
Contributor Author

rixx commented Apr 29, 2019

Thank you for your explanation. I can modify the PR to include your suggested changes, but I'm not clear on where/how to pin pyyaml to a minimum version, apart from the documentation. Do we have other places where we check for optdepdend versions?

@ngnpope
Copy link
Member

ngnpope commented Apr 29, 2019

I wouldn't make the change at this stage unless the the community agrees to pin the version. So for now just take my comment as being the correct explanation as to why this is resolved.

@rixx
Copy link
Contributor Author

rixx commented Apr 29, 2019

I'll add it to the commit message, for correctness' sake 😉

The fix for this issue was provided in 24b82cd, particularly the
changes made to django/core/serializers/pyyaml.py
Thank you, Nick Pope, for the fix and the review.
@felixxm
Copy link
Member

felixxm commented May 3, 2019

I checked and this has been originally fixed in 5bc3123.

@felixxm
Copy link
Member

felixxm commented May 3, 2019

This is already tested in SerializersTestBase.test_deterministic_mapping_ordering().

@rixx rixx closed this May 3, 2019
@rixx rixx deleted the ticket_11929 branch May 3, 2019 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants