Key ordering with locale, take #2

[wolfgangwalther]

Compared to the PR before, this does not set a default locale at all. If the locale config option is not used, no setlocale() is executed, so no error possible.

[coveralls]

Coverage decreased (-0.07%) to 97.867% when pulling 4f6975d on wolfgangwalther:key-ordering-with-locale into 0fceca2 on adrienverge:master.

[adrienverge]

Can you briefly explain why you added this? Is the cleanup on line 334 redundant?

[wolfgangwalther]

The cleanup is still needed after the whole test ran.

Setting the locale to en_US is just a test for locale availability. But because the default without locale option (as in the first of the for test .runs below) doesn't set any locale, that en_US would still be active. Therefore we need to reset that to it's initial state first, then run the 2 tests with default settings, then the 2 with locale setting, then cleanup.

[adrienverge]

Thanks!

I understood the second one (line 339), but I thought the cleanup wasn't needed, then. Actually it's still needed because yamllint calls locale.setlocale() too (in cli.py).

Could you add a tiny comment above self.addCleanup(), just to make sure we remember it? E.g. # Clean up after yamllint called locale.setlocale() in cli.py:

[wolfgangwalther]

I added some comments and moved the addCleanup to a more appropriate spot regarding the flow in this test.

[wolfgangwalther]

I now understand more how the whole locale module works, so I improved this test case (unrelated to my changes) as well. This should now work for any available UTF-8 locale, I guess - and also reset to the "default" from before better then with getlocale.

[adrienverge]

Interesting: 'C.UTF-8' → (None, 'UTF-8')

Thanks!

[adrienverge]

👍

[adrienverge]

Interesting: 'C.UTF-8' → (None, 'UTF-8')

Thanks!

[wolfgangwalther]

Interesting: 'C.UTF-8' → (None, 'UTF-8')

Only.... that it doesn't do what I thought it would :/

Did a little more testing - it's basically the same as C.UTF-8. I still think the test is better this way with the skipTest and the proper clean-up, though.

I will try to add some environment without locales to Travis to prevent the whole regression from happening again.

[ssbarnea]

While I am really interested about key ordering and I want to add a feature like this to ansible/ansible-lint#578 the current implementation worries me a little because it does not take in consideration that different yaml files would need custom ordering.

One good example is the fact that Ansible files would want to have the "name" key the first key, and that is clearly not alphabetical. Using alphabetical ordering would make no sense at all.

I know this is a complex issue but it would be nice to have a bug where we can plan.

As I use yamllint in tandem with ansible-lint, I am also considering integrating them, so users would not have to run both.

[wolfgangwalther]

While I am really interested about key ordering and I want to add a feature like this to ansible/ansible-lint#578 the current implementation worries me a little because it does not take in consideration that different yaml files would need custom ordering.

One good example is the fact that Ansible files would want to have the "name" key the first key, and that is clearly not alphabetical. Using alphabetical ordering would make no sense at all.

I am currently using yaml files for translations - in that use-case it makes perfect sense to sort the keys alphabetical.

I agree, though, that e.g. for various types of config files, custom ordering rules would be great (I'm thinking docker-compose.yml or various CI config files right now). I am not sure whether that could/should be implemented in the existing key-ordering rule or whether that should be a different rule. It might make more sense to create a different rule for it, to keep things easy to implement.

As for different files needing different rules: Not sure whether that's possible right now, I guess not. That might be a completely independent issue: Defining rules individually by file name (or some kind of pattern).

I know this is a complex issue but it would be nice to have a bug where we can plan.

You could just create an issue for it? ;)

Or better - a PR to implement it?

[ssbarnea]

@wolfgangwalther It is more complex than this, read the linked ticket and preferably see if you can help YAML spec owners to help us. A comment on the spec ticket could help, even a message on their freenode channel #yaml-dev.

[wolfgangwalther]

If yaml schemas would become a thing, that would be a great way to apply different configs to files.

However, in the meantime you could still implement a "custom-key-order" rule already, as it doesn't depend on different configs directly. This way you would already be half-way there.