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

Allow to turn off sorting keys in Dumper #143

Closed
wants to merge 1 commit into from

Conversation

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 16, 2018

See issue #110

This allows to do:

output = yaml.dump(data, sort_keys=False)

The default is True because of backwards compatibility.

This can be useful for python >= 3.7 to preserve key order. Also not sorting will probably be faster.

I didn't add a test because I'm not yet familiar with testing in python
and couldn't find out how the pyyaml test suite works.

Also i don't know where to document that.

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Mar 16, 2018

Travis tests are failing. I need to find out how to run the tests locally. Any hints for a python beginner?

Loading

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Mar 16, 2018

thanks @ingydotnet for python setup.py test
I pushed a fix for cyaml.py, tests are passing now for python > 2.6

Loading

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Mar 17, 2018

I started to write a test for this. Will probably push it tomorrow

Loading

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Apr 15, 2018

rebased to master, tests now including 3.7-dev, and 2.6 is passing

Loading

@perlpunk perlpunk force-pushed the dumper-sortkeys branch from 4a74967 to 6aeacf0 Jul 1, 2018
@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Jul 1, 2018

Rebased to current master, squashed commits.
Tests are passing (and now include the libyaml tests also)

Loading

@dsposito
Copy link

@dsposito dsposito commented Jul 6, 2018

This is great! Looking forward to this being merged! 🚀

Loading

@perlpunk perlpunk removed the request for review from sigmavirus24 Jul 6, 2018
@jasonrhaas
Copy link

@jasonrhaas jasonrhaas commented Jul 23, 2018

Any progress on this PR?

Loading

@dsposito
Copy link

@dsposito dsposito commented Aug 6, 2018

@jasonrhaas I'm hoping this eventually gets merged to source but for now oyaml does the trick.

Loading

@Ark-kun
Copy link

@Ark-kun Ark-kun commented Oct 3, 2018

Please, merge this. I've spent countless hours trying to work around this.
P.S. No, oyaml is not a solution for us, since our team is already using pyyaml and does not want to change all code files.
P.P.S. No, OrderedDict is not a solution, because it radically changes the output and yaml types. (mapping to sequence)

Loading

@dilumr
Copy link

@dilumr dilumr commented Oct 24, 2018

I too am interested is this. Would like to use in a feature where a tool slightly modifies YAML files that are checked into source. Would love the edits to look slight. The YAML files are initially ordered by hand, to expecting keys to be alphanumerically sorted is unrealistic; humans tend to group keys by purpose and/or importance.

Loading

@ingydotnet ingydotnet added this to Approved in 5.1 Release Nov 8, 2018
@ingydotnet ingydotnet moved this from Approved to Added in 5.1 Release Nov 8, 2018
@gatopeich
Copy link

@gatopeich gatopeich commented Nov 20, 2018

Dear Perl punk (since you seem the only maintainer left here), leaving "sorted" by default is still wrong.

Sorting the keys is a superfluous operation wrt the YAML spec. It is also anti-pythonic, and goes against the most basic programming principle of not doing needless things.

Now it is obvious from the comments that devs using PyYAML for actual work are negatively impacted by this anti-feature. Like in my case, it renders this library unusable for their projects.

Whoever wants the keys sorted should be doing it at the .yaml source. That is the whole point of defining interfaces in YAML.

"Backwards compatibility" feels like a weak excuse here.

Please listen to the community and be honest here.

Loading

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Nov 21, 2018

@gatopeich this is a pull request. It's a suggestion.

I don't decide anything, I can just help making decisions.

I agree that for python versions that keep the original order, sort_keys=False would be the better default.
For older versions I think that default wouldn't make sense, because getting an arbitrary sort order when dumping is less helpful than sorted keys. Plus it's not backwards compatible.

Would it make sense to make the default depend on the python version?
Thoughts?

Loading

@gatopeich
Copy link

@gatopeich gatopeich commented Nov 21, 2018

Would it make sense to make the default depend on the python version?

Okay. Then let's apply same default as for json.dumps(): sort_keys=True until Python 2.6, False since v2.7.

Loading

@ofek
Copy link

@ofek ofek commented Dec 25, 2018

Can this be merged now?

Loading

@rr-
Copy link

@rr- rr- commented Jan 27, 2019

Would it make sense to make the default depend on the python version?
Thoughts?

If you're concerned about portability, it's okay to have it default to True. The important bit here is to be able to customize it at all, which we're still unable to.

(I think having a package changing its behavior after seemingly transparent Python upgrade breaks the principle of least astonishment, although personally I too would like to see it default to False.)

Loading

@perlpunk
Copy link
Member Author

@perlpunk perlpunk commented Feb 17, 2019

Closing, recreated based on current master: #254

Loading

@perlpunk perlpunk closed this Feb 17, 2019
@perlpunk perlpunk deleted the dumper-sortkeys branch Mar 18, 2019
enzbang added a commit to enzbang/e3-aws that referenced this issue May 9, 2019
enzbang added a commit to enzbang/e3-aws that referenced this issue May 9, 2019
enzbang added a commit to enzbang/e3-aws that referenced this issue May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.1 Release
Added to release/5/1 branch
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants