-
Notifications
You must be signed in to change notification settings - Fork 264
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 comments that match any previous level of indentation #141
Comments
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698 --HG-- extra : moz-landing-system : lando
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698
This would need a new option on the |
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698 --HG-- extra : rebase_source : 15a8b2134ab452340049084b9702daf12229842b extra : source : 4833b89bb969cf1709eba8f8960136c85e750d94
This other case also fails: config:
- property1: some value
property2: some other value
# comment Also gives a |
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698 UltraBlame original commit: 4833b89bb969cf1709eba8f8960136c85e750d94
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698 UltraBlame original commit: 4833b89bb969cf1709eba8f8960136c85e750d94
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698 UltraBlame original commit: 4833b89bb969cf1709eba8f8960136c85e750d94
Due to adrienverge/yamllint#141 we can't indent the comments to the appropriate level. Differential Revision: https://phabricator.services.mozilla.com/D9698
I would say this counts as a bug in indentation rule as it creates false positive like below even when using code-commenting support in your editors.
Current behavior forces people to miss-indent they temporary disable YAML code, so when they use editor to uncomment it, it will be wronly indented. |
This bug seems to become more annoying as it forces users to wrongly change indentation of commented lines, introducing bugs. In the example below line 6 is wrongly indicated with comments-indentation when that is the Most editors do have shortcuts for comment/uncomment and they do keep indentation correctly. When yamllint runs, it gives a false positive about indentation. Most users will wrongly change indentation to make the linter happy. Big mistake, as this introduce a bug when the lines are uncommented. This can easily pass CI and code0review, especially when lists are longer, and have disastrous effects. I know for sure one place where current behavior invites people to mistakenly move
So, please accept this proposal which is almost two years old. Having a rule about comment indentation is good and we should not be forced to disable it because is partially broken. Lets just fix it. |
@ssbarnea as always, the tone of your comments is a bit annoying. This issue is known, and in the first reply to this thread I wrote "proposals and contributions are welcome!"
So what are you waiting for? PS: It's not a "bug", like you say. It's intended to work like that. If you're unhappy, please disable the rule (it's very easy and yamllint will keep working), or contribute (positively) to this project. |
@adrienverge Your last comment gave me the impression that you do not want the default behavior to be changed. If true, I doubt someone would create a PR that adds another option that is disabled by default, not when it is so easy to just disable the entire If it would be about my personal projects it would not be big deal, I already did that. The reason why I supported this change is related to scaling good practices. It is really hard to promote a linter that does invite users to break their code. I mention this because at least two different people asked me that. What I am waiting for? I am waiting for a confirmation from the project lead regarding what kind of change would be found acceptable. |
@ssbarnea you're talking about "a bug", yamllint "inviting users to break their code", and "disastrous effects". Let's take a step back and stop dramatizing. You want to change the default behavior of this rule to match your personal expectations, as well as the 2 people you've talked to (and maybe many other people). The problem is that what you call "false positives about indentation" is what some other people expect as true positives -- because your good practices are not always the other ones' good practices. Changing the default behavior would break the rule for these users. Instead, we could use an opt-in configuration (proposal welcome) to achieve your good practice. |
Fixed yamllint error excluding following rules. - line-length: Default limit 80 is too small - comments-indentation: This rule is being discussed at adrienverge/yamllint#141 and adrienverge/yamllint#384
Fixed yamllint error excluding following rules. - line-length: Default limit 80 is too small - comments-indentation: This rule is being discussed at adrienverge/yamllint#141 and adrienverge/yamllint#384
Comments that match a previous level of indentation should be allowed, at the end of that level of indentation.
A minimal example that I think should lint successfully:
gives
The text was updated successfully, but these errors were encountered: