[BUG] Default setting (enable) of empty-values & octal-values does nothing

[myii]

There appears to be a contradiction between the meaning of enable in the default settings and the actual end result.

empty-values

yamllint/yamllint/conf/default.yaml

Line 21 in b62b424

empty-values: enable

yamllint/yamllint/rules/empty_values.py

Lines 78 to 79 in 0f073f7

	DEFAULT = {'forbid-in-block-mappings': False,
	'forbid-in-flow-mappings': False}

octal-values

yamllint/yamllint/conf/default.yaml

Line 29 in b62b424

octal-values: enable

yamllint/yamllint/rules/octal_values.py

Lines 69 to 70 in 0f073f7

	DEFAULT = {'forbid-implicit-octal': False,
	'forbid-explicit-octal': False}

---

In both cases, the only way to get these working is to specify them in the .yamllint file:

rules:
  empty-values:
    forbid-in-block-mappings: true
    forbid-in-flow-mappings: true
  octal-values:
    forbid-implicit-octal: true
    forbid-explicit-octal: true

What is enable supposed to do? Otherwise, isn't the default actually disable in both cases, with default.yaml needing updating (with the knock-on affect to the documentation)?

[adrienverge]

Hi @myii, thanks for shedding light on this.

I agree the default conf for empty-values and octal-values is poorly implemented.
I agree the configuration options forbid-in-block-mappings, forbid-in-flow-mappings, forbid-implicit-octal and forbid-explicit-octal should default to true.

The problem is that it's not easy to change default options: it might break existing configurations and start generating errors in software projects that don't expect it. Including large scale projects (Ansible, OpenStack, Docker...) that can't allow that.
Do you see any potential problems if we switch all these 4 options to true and turn the two rules to disable in the default conf at the same time?

[myii]

@adrienverge Thanks for the response. For the time being, I believe it's sufficient to just do one of the things you've mentioned. So either:

1. Switch the four options to True; or
2. Update the default profile to show disable instead of enable.

The ideal answer is number 1 but it could cause serious issues in projects that are out there. So number 2 is the right option for the time being, so that at least users are informed that these settings are disabled by default. So essentially, all that needs to be changed for now in default.yaml:

@@ -18,7 +18,7 @@
   document-start:
     level: warning
   empty-lines: enable
-  empty-values: enable
+  empty-values: disable
   hyphens: enable
   indentation: enable
   key-duplicates: enable
@@ -26,7 +26,7 @@
   line-length: enable
   new-line-at-end-of-file: enable
   new-lines: enable
-  octal-values: enable
+  octal-values: disable
   quoted-strings: disable
   trailing-spaces: enable
   truthy:

What do you think?

[adrienverge]

Yeah, 1. cannot be done, it would break many users' configurations.

If we disable these rules by default, why not switching options to true at the same time, so that users don't get confused when enabling the rule?

[myii]

Apologies, maybe I misunderstood you. Which options should be switched to True?

[adrienverge]

My proposal is to switch from:

• yamllint/conf/default.yaml:

  empty-values: enable 

  yamllint/rules/empty_values.py:

  DEFAULT = {'forbid-in-block-mappings': False,
             'forbid-in-flow-mappings': False}

to:

• yamllint/conf/default.yaml:

  empty-values: disable 

  yamllint/rules/empty_values.py:

  DEFAULT = {'forbid-in-block-mappings': True,
             'forbid-in-flow-mappings': True}

(and the same for octal-values)

The goal is:

• for users that just have the default conf (or any extension of default) not to notice the change,
• for users that already enabled options of empty-values or octal-values not to notice the change,
• for future users that would want to use empty-values: enable (like you) to have all options enabled by default.

[myii]

@adrienverge Ah, OK. That sounds like an excellent proposal, which covers the existing uses of yamllint as well as making it easier to enable these settings for new/all users. The only thing is that existing users who are assuming that these values are enabled by default, still won't know that they aren't enabled! Is there any way we can notify them of the change?

[adrienverge]

The only thing is that existing users who are assuming that these values are enabled by default, still won't know that they aren't enabled! Is there any way we can notify them of the change?

I agree, but I don't see the fact that these rules are disabled by default as a problem:

• it was already the case before (because of options forbid-*: false),
• there are others rules disabled by default (quoted-strings, document-end, key-ordering) because they are considered too strict,
• it will appear in the docs: https://yamllint.readthedocs.io/en/stable/configuration.html#default-configuration

Would you like to contribute this change?

[myii]

I agree, but I don't see the fact that these rules are disabled by default as a problem:

@adrienverge It's not a major issue but there is a problem of expectation: users (like myself) believe that the rules are enabled, since that's what we read in the documentation. I only realised when it was never showing errors which I knew existed in the codebase. Others will be out there who will remain oblivious, thinking it's enabled by default. Perhaps some appropriate message to make them aware of this, that they actually need to add this to their configuration files:

  empty-values: enable
  octal-values: enable

• there are others rules disabled by default (quoted-strings, document-end, key-ordering) because they are considered too strict,

I agree that some rules should be opt-in. These would have been nice if they were working by default but I fully support that yamllint should remain backwards-compatible.

• it will appear in the docs: https://yamllint.readthedocs.io/en/stable/configuration.html#default-configuration

Not sure existing users will check that again after having based their configurations on that. But there's not much more that can be done. Perhaps a specific notice under empty-values and octal-values sections, for a period of time until existing users can catch up?

Would you like to contribute this change?

Sure, I'll put this through soon.

[myii]

@adrienverge I think this is going to need a little more thought:

yamllint/tests/rules/test_octal_values.py

Line 43 in 579a975

self.check('explicit-octal: 0o10', conf)

yamllint/tests/rules/test_octal_values.py

Line 64 in 579a975

self.check('implicit-octal: 010', conf)

$ python -m unittest -v tests.rules.test_octal_values
test_disabled (tests.rules.test_octal_values.OctalValuesTestCase) ... ok
test_explicit_octal_values (tests.rules.test_octal_values.OctalValuesTestCase) ... FAIL
test_implicit_octal_values (tests.rules.test_octal_values.OctalValuesTestCase) ... FAIL

======================================================================
FAIL: test_explicit_octal_values (tests.rules.test_octal_values.OctalValuesTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/scratch/GitHub/yamllint/tests/rules/test_octal_values.py", line 103, in test_explicit_octal_values
    self.check('implicit-octal: 010', conf)
  File "/home/scratch/GitHub/yamllint/tests/common.py", line 53, in check
    self.assertEqual(real_problems, expected_problems)
AssertionError: Lists differ: [1:20: forbidden implicit octal value "010" (octal-values)] != []

First list contains 1 additional elements.
First extra element 0:
1:20: forbidden implicit octal value "010" (octal-values)

- [1:20: forbidden implicit octal value "010" (octal-values)]
+ []

======================================================================
FAIL: test_implicit_octal_values (tests.rules.test_octal_values.OctalValuesTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/scratch/GitHub/yamllint/tests/rules/test_octal_values.py", line 82, in test_implicit_octal_values
    self.check('explicit-octal: 0o10', conf)
  File "/home/scratch/GitHub/yamllint/tests/common.py", line 53, in check
    self.assertEqual(real_problems, expected_problems)
AssertionError: Lists differ: [1:21: forbidden explicit octal value "0o10" (octal-values)] != []

First list contains 1 additional elements.
First extra element 0:
1:21: forbidden explicit octal value "0o10" (octal-values)

- [1:21: forbidden explicit octal value "0o10" (octal-values)]
+ []

----------------------------------------------------------------------
Ran 3 tests in 0.162s

FAILED (failures=2)

[adrienverge]

It's not a major issue but there is a problem of expectation: users (like myself) believe that the rules are enabled

I get your point, and agree it can be confusing. However this is not specific to empty-values and octal-values, but all disabled rules in https://github.com/adrienverge/yamllint/blob/7359785/yamllint/conf/default.yaml.

Perhaps a specific notice under empty-values and octal-values sections, for a period of time until existing users can catch up?

I'm not sure it's really needed, because existing users won't go check there.

However, what could be useful is adding a note under all rules that are disabled by default.

[myii]

@adrienverge The rest of the rules that are set to disable are not a surprise, since we saw that when we first read the docs. Whereas empty-values and octal-values are a surprise, because they weren't actually doing what we were expecting. It's the errata concept that I'm trying to explain -- the rest of the disabled values don't suffer from that.

In any case, maybe no-one will notice. Then again, I noticed something was wrong and the first place I came back to was the documentation, before rolling up my sleeves to jump into the code!