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

disable-line rule:comments-indentation does not seem to work? #343

Open
yurtesen opened this issue Nov 3, 2020 · 7 comments
Open

disable-line rule:comments-indentation does not seem to work? #343

yurtesen opened this issue Nov 3, 2020 · 7 comments

Comments

@yurtesen
Copy link

yurtesen commented Nov 3, 2020

Using yamllint 1.25.0. I have a line like below:

    # yamllint disable-line rule:comments-indentation
    # Todo: Set this value

I am getting warning comment not indented like content (comments-indentation) on the # yamllint ... line!

If I use

    # yamllint disable rule:comments-indentation
    # Todo: Set this value
    # yamllint enable rule:comments-indentation

Then everything is fine, no errors are reported.

Not sure what my be causing this? Shouldn't both work the same?

$ cat test.yaml 
    # yamllint disable-line rule:comments-indentation
    # Todo: Set this value
$ yamllint test.yaml 
test.yaml
  1:5       warning  comment not indented like content  (comments-indentation)
@adrienverge
Copy link
Owner

Hello,

Your snippet shows that the problem is on line 1 (not 2): the first comment (# yamllint disable-line ...) is not indented like previous content.

If you use this, it shouldn't complain:

# yamllint disable-line rule:comments-indentation
    # Todo: Set this value

Using # yamllint disable (instead of # yamllint disable-line) clears errors on the whole block of lines, that's why it does the job.

I recommend using # yamllint disable 👍

@yurtesen
Copy link
Author

yurtesen commented Nov 4, 2020

@adrienverge yes that is exactly my point. Why the #yamllint disable-line is not immune to whatever it is disabling meanwhile #yamllint disable is immune under same conditions. Why?

Meanwhile you suggested #yamllint disable and it is immune. But #yamllint enable is also NOT immune. For example when I make a code block between #yamllint disable and #yamllint enable I would like these 2 statements to be on same depth. It is a block of code between these statements after all. But it is not possible either.

$ cat test.yaml 
---
test1:
  # yamllint disable rule:comments-indentation
  test100:
    - test101
    - test102
  # yamllint enable rule:comments-indentation
$ yamllint test.yaml 
test.yaml
  7:3       warning  comment not indented like content  (comments-indentation)

I am not sure if it is me but having disable rule on one depth and enable rule on another depth looks strange to me....hmm?

@adrienverge
Copy link
Owner

It's not shocking to me, as # yamllint disable-line ... is supposed to disable errors starting from the next line.

However if you have a better (and consistent) behavior to propose, feel free!

If you do so, please take into account that users may want # yamllint directives to be reported if wrongly indented:

foo: 1
bar: the following line is wrongly indented and I want it to be reported:
          # yamllint disable-line rule: key-duplicates
bar: key-duplicates

@yurtesen
Copy link
Author

yurtesen commented Nov 4, 2020

@adrianverge I see your point, but doesn't same concern apply for # yamllint disable rule:comments-indentation? It seems to be happily immune to being on wrong depth. With same logic why the following seems to be accepted? :)

foo: 1
bar: the following line is wrongly indented and I want it to be reported:
          # yamllint disable rule:comments-indentation
bar: key-duplicates

Furthermore, this also does not work when it is on the same line with comment (probably due to double comment?). Even though there is an example about line-length using disable-line., in manual: https://yamllint.readthedocs.io/en/stable/disable_with_comments.html

$ cat test.yaml 
---
  # Todo: Set this value # yamllint disable-line rule:comments-indentation
$ yamllint test.yaml 
test.yaml
  2:3       warning  comment not indented like content  (comments-indentation)

I do not know what would be the best way to deal with this. I am pretty new to yaml-lint :) I think the issue is maybe even very specific to comments-indentation. I think the best solution would be disable-line behaving similar to disable and disable the rule on its own line also. If I am using this directive to avoid some wrong indentation, it is likely that directive itself would be indented incorrectly....

About # yamllint enable.... it should always be immune to comments indentation. We can't really expect it to be aligned with content. Just like a block of code, it should be aligned with # yamllint disable instead. Doesn't that make sense?

I may be wrong, I did not use yaml-lint a lot and by now I solved this problem by completely eliminating the comment line :) I guess one way would be leaving this ticket open and see if others who has similar problems would comment also. But I think not many are using this feature, It seems more common to just use .yamllint so maybe nobody hits this issue for long time.

@adrienverge
Copy link
Owner

Hello @yurtesen,

I think the best solution would be disable-line behaving similar to disable and disable the rule on its own line also. If I am using this directive to avoid some wrong indentation, it is likely that directive itself would be indented incorrectly....

Sounds fair. That's not how it was intended to work, but your modification proposal seems to solve your problem without creating new ones. Contributions are welcome! (they must be clean and come with tests)

About # yamllint enable.... it should always be immune to comments indentation. We can't really expect it to be aligned with content. Just like a block of code, it should be aligned with # yamllint disable instead. Doesn't that make sense?

I'm not sure about this. We may want to expect it to be aligned with content. It may not be necessarily aligned with # yamllint disable. For instance if disable starts before a mapping (identation 0) and enable ends inside the mapping (indentation 4).

@yurtesen
Copy link
Author

yurtesen commented Nov 5, 2020

I am throwing out some ideas but I think right now I don't have time to implement anything at this point. I already proceeded to some other tasks and I am already behind on several other tasks :( but if I have problems with yamllint again I can try to divert some time to help out with some improvements.

I'm not sure about this. We may want to expect it to be aligned with content. It may not be necessarily aligned with # yamllint disable. For instance if disable starts before a mapping (identation 0) and enable ends inside the mapping (indentation 4).

Comments are tricky. In documentation, there is an example:

# yamllint disable rule:colons
- Lorem       : ipsum
  dolor       : sit amet,
  consectetur : adipiscing elit
# yamllint enable rule:colons

Yet this gives warning

items:
  # yamllint disable rule:colons
  - Lorem       : ipsum
    dolor       : sit amet,
    consectetur : adipiscing elite
  # yamllint enable rule:colons

But this one is OK

items:
  # yamllint disable rule:colons
  - Lorem       : ipsum
    dolor       : sit amet,
    consectetur : adipiscing elite
    # yamllint enable rule:colons

But this give warning

# yamllint disable rule:colons
- Lorem       : ipsum
  dolor       : sit amet,
  consectetur : adipiscing elite
  # yamllint enable rule:colons

All I am saying is that the way this works is quite confusing and unpredictable. Also probably unfixable due to complex combinations which can be created. Look at this, it passes fine...

items:
  # yamllint disable rule:colons
  - Lorem       : ipsum
    # yamllint enable rule:comments-indentation
    dolor       : sit amet,
    consectetur : adipiscing elite
    # yamllint enable rule:colons
  # yamllint disable rule:comments-indentation

although very difficult ot read IMHO.

I think things would have been clearer if # yamllint enable/disable directives could work like a block with a start and end marker.... This way check would be matching depth of start and end markers only.

Something like this is much more readable, although not as flexible :

items:
  # yamllint begin disable rule:colons
  - Lorem       : ipsum
    # yamllint begin rule:comments-indentation
    dolor       : sit amet,
    consectetur : adipiscing elite
    # yamllint end            
  # yamllint end   

Like I said, I am just throwing out some ideas. The comments are tricky...

@naanlizard
Copy link

Furthermore, this also does not work when it is on the same line with comment (probably due to double comment?). Even though there is an example about line-length using disable-line., in manual: https://yamllint.readthedocs.io/en/stable/disable_with_comments.html

$ cat test.yaml 
---
  # Todo: Set this value # yamllint disable-line rule:comments-indentation
$ yamllint test.yaml 
test.yaml
  2:3       warning  comment not indented like content  (comments-indentation)

FWIW line-length doesn't seem to be disable-able, for whatever reason, still, and this is the only reference I can find (maybe) to that bug.

registry_nginx['proxy_set_headers'] = {"X-Forwarded-Proto" => "https","X-Forwarded-Ssl" => "on", "Host" => "registry.hostname.com"} # yamllint disable-line rule:line-length

user@host:~/$ docker run --rm -it -v $(pwd):/data cytopia/yamllint .

./home/gitlab/docker-compose.yml

  24:81     error    line too long (190 > 80 characters)  (line-length)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants