adrienverge / yamllint

A linter for YAML files.
GNU General Public License v3.0
2.83k stars 269 forks source link

Compact nested block sequence ignores indentation configuration #250

Open styurin opened 4 years ago

styurin commented 4 years ago

Compact nested block sequences are always formatted with the same indentation ignoring the configuration for sequences.

For example, for configuration indentation: {spaces: 3, indent-sequences: true} the following input will result in an error ("too many spaces after hyphen (hyphens)"):

object:
   k1:
      - a
      - b
   k4:
      -  - item1
         - item1

This text needs to be changed to the following to pass the linter:

object:
   k1:
      - a
      - b
   k4:
      - - item1
        - item1

Note that there is one whitespace between two hypens, not two whitespaces as before.

According to the spec, for such sequences the first hypen should be considered as part of the indentation of the nested block. In this case the indentation is 3.

Spec for reference: https://yaml.org/spec/1.2/spec.html#id2797382

adrienverge commented 4 years ago

Hello @styurin, thanks for the report.

I don't see anything abnormal here, I confirm that the rule tries to enforce the second snippet. What's after an hyphen + a space is considered the item value by yamllint. I understand you see the space(s) as indentation, to be honest why not, but I believe most users see this space as a mandatory separator that should be as small as possible, because they have hyphens: {max-spaces-after: 1} in conf.

In the spec, I understand the sentence that you mention "both the “-” indicator and the following spaces are considered to be part of the indentation of the nested collection" in the way "both the “-” indicator and the following spaces set which column should be used if the item is continued on multiple lines".

In your case, I think what makes most sense is using this conf:

rules:
  indentation: {spaces: 3, indent-sequences: true}
  hyphens: {max-spaces-after: 2}

or:

rules:
  indentation: {spaces: 3, indent-sequences: true}
  hyphens: disable

What do you think?

styurin commented 4 years ago

Changing hypens configuration would affect all of the cases with hypens which may not be desirable.

This is a special case that is shown in the example 8.15 in the spec, it has a comment that says "Compact sequence". The linter enforces this special case as it is written in the spec and there are tests cases that validate this scenario, but IMO that is a poor example because it's using indentation 2 and with that it's hard to see where the indentation is applied. If you look at the example you would see a block that is starting at a position 0 on the second line and at position 1 on the first line with the hypen taking part of indentation.

With indentation 3 that example would be:

-  - one # Compact
   - two # sequence

and with four:

-   - one # Compact
    - two # sequence

Here's the part of the spec that describes that:

The compact notation may be used when the entry is itself a nested block collection. In this case, both the “-” indicator and the following spaces are considered to be part of the indentation of the nested collection.

In my initial example item1 and item2 form a nested collection which indentation should be 3 according to the configuration, but the linter is enforcing indentation 2.

I understand you see the space(s) as indentation, to be honest why not, but I believe most users see this space as a mandatory separator that should be as small as possible

I don't really have a preference here, it's just the spec explicitly describing this situation and it seems that it's not processed correctly. I'd prefer adhere to the spec since its purpose is to avoid these interpretations that make it harder to use different tools that operate with the same data.

adrienverge commented 4 years ago

I'm not as sure as you that the spec explicitly describes this situation. I agree multiples spaces after - are allowed. But is it recommended to add spaces to align on indentation? Look at example 8.14: indentation is denoted with ··, but after - there is a simple (and unique) .

Another argument: by following your indentation logic, but using spaces: 1 instead of 3, we would get either:

object:
 k1:
  -a
  -b
 k4:
  --item1
   -item1

or:

object:
 k1:
 - a
 - b
 k4:
 - - item1
   - item1

... which is obviously invalid in case a, and unaligned in case b. This goes in favor of always using one space after -, for logic and consistency.


Anyway, most of yamllint rules enforce more than the spec, which is the case here with hyphens: {max-spaces-after: 1}. I think that's what most users would expect.

That being said, your use case looks valid (although rare: it's the first time I see it), so it could be added to yamllint using an option (to avoid breaking current behavior for existing users).

However there's something I don't understand: in your example, why don't you want a and b to be indented by 3 spaces? To be consistent, I would expect:

object:
   k1:
      -  a
      -  b
   k4:
      -  - item1
         - item1
styurin commented 4 years ago
object:
 k1:
  -a
  -b
 k4:
  --item1
   -item1

This is not correct since a node should be separated by a whitespace from -.

object:
 k1:
 - a
 - b
 k4:
 - - item1
   - item1

According to spec I'd expect it to be the following with indentation 1:

object:
 k1:
 - a
 - b
 k4:
 - - item1
  - item1

This doesn't look pretty, but would work according to the spec.

Anyway, most of yamllint rules enforce more than the spec, which is the case here with hyphens: {max-spaces-after: 1}. I think that's what most users would expect.

Providing more features than described in the spec is definitely valuable for users. My main point about the spect is that not following it makes it harder to combine different tools.

I encountered this problem while investigating that another tool (ruamel.yaml) does not follow the spec which results in failures in yamllint. That tool doesn't handle compact sequences at all (yamllint at least handles them). You can check my report here: https://sourceforge.net/p/ruamel-yaml/tickets/346/

However there's something I don't understand: in your example, why don't you want a and b to be indented by 3 spaces?

That's fine to me, though as I understand that would require hyphens: {max-spaces-after: 2} which in turn would allow to have something like this:

object:
   k1:
      -  a
      - b
adrienverge commented 4 years ago

My main point about the spect is that not following it

Once more, I don't think yamllint "doesn't follow the spec". I understand you have your own interpretation of the spec, I respect that and it seems valid, but again: