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

Neuter min space requirement for inline comments #1821

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jan 21, 2022

Update our yamllint config defaults to disable the requirement of adding two spaces before each inline comments. This does not affect users that already set a different value in their yamllint config files.

This change will not produce extra errors after is applied, it will only reduce the number of errors reported in general as foo: bar # that is cool! will no longer complain about missing an additional space, using one or two will be fine.

The expected behavior is that ansible-lint will no longer report problems like:

warning  too few spaces before comment  (comments)

This will improve the compatibility with various code formatters, as mentioned in #1820

Update our yamllint config defaults to disable
the requirement of adding two spaces before
each inline comments. This does not affect users
that already set a different value in their yamllint config files.

This will improve the compatibility with various code formatters, as mentioned in ansible#1820
@ssbarnea ssbarnea marked this pull request as ready for review January 21, 2022 15:31
@ssbarnea ssbarnea requested a review from a team as a code owner January 21, 2022 15:31
@ssbarnea ssbarnea requested review from relrod, ganeshrn and priyamsahoo and removed request for a team January 21, 2022 15:31
Copy link
Contributor

@greg-hellings greg-hellings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, wholeheatedly.

Copy link
Contributor

@cidrblock cidrblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

Copy link
Contributor

@tadeboro tadeboro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change mean that previously "green" YAML files will now become "red"? If yes, then I am -1 on this change during the 5.x.y series of releases. Breaking people's CI/CD setups is not something we should do lightly.

And I know introducing new rules can also "break" the CI/CD, but those situations are different since in most cases they indicate that something can be improved. Changing comment formatting brings no direct benefit to linter users.

@ssbarnea
Copy link
Member Author

I did not see any breakage reported by eco job but I do realise that someone could be surprised by the new setup. Maybe we can do this in two steps, first disabling that check and delaying the new preference for a major version change.

@ssbarnea ssbarnea changed the title Remove min space requirement for inline comments Reduce min space requirement for inline comments Feb 2, 2022
@ssbarnea ssbarnea changed the title Reduce min space requirement for inline comments Neuter min space requirement for inline comments Feb 2, 2022
@ssbarnea ssbarnea merged commit 6312bb0 into ansible:main Feb 2, 2022
@ssbarnea ssbarnea deleted the fix/comments branch February 2, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants