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

chore(lint): increase line-length max value #219

Merged

Conversation

rebornplusplus
Copy link
Member

@rebornplusplus rebornplusplus commented Apr 9, 2024

line-length errors are now warnings.

This PR increases the maximum length of a line in the yaml files.

Relevant discussions at #142.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

I think this is dangerous.

You are opening the doors for contributors to start writing huge lines, including in places where such shouldn't happen (like contents).

This seems to be driven by a problem in a mutation script in #142 . For that, I'd rather:

  • just fix the mutation script. I get that it is a copy&paste, but if that copy&paste is "broken" that's not an excuse for us to accept it like that.
    • if the issue however is that the YAML format is forcing an indentation that is causing that copy&paste to be longer than acceptable, then let's just increase the max value
  • another option is fiddling with the yamllint configuration. I'm not sure if it is possible, but ideally, we could be permissive about some blocks of yaml. E.g. you could allow the "level: warning", but only for the "mutation" blocks. Again, not sure if that's possible
    • or, don't lint # comments

@Meulengracht
Copy link
Member

I think the issue in my PR is that the linter does not account for the indentation made before the contents itself. The length of the contents itself does not exceed the 80 characters.

@cjdcordeiro
Copy link
Collaborator

I think the issue in my PR is that the linter does not account for the indentation made before the contents itself. The length of the contents itself does not exceed the 80 characters.

That's good to hear, as it makes me more confident about simply increasing the max value to accommodate the expected indentations (if no cleaner yamllint-specific remediation is available)

The maximum length of a line is now 100.
@rebornplusplus rebornplusplus changed the title chore(lint): line-length errors are now warnings chore(lint): increase line-length max value May 2, 2024
@rebornplusplus
Copy link
Member Author

Updated the PR. Set the limit to 100 to account for any indentation spaces.

@rebornplusplus rebornplusplus requested a review from cjdcordeiro May 2, 2024 12:23
@cjdcordeiro cjdcordeiro removed their assignment May 16, 2024
@rebornplusplus rebornplusplus self-assigned this May 16, 2024
@rebornplusplus rebornplusplus removed their assignment May 16, 2024
@cjdcordeiro cjdcordeiro requested a review from linostar May 16, 2024 13:26
@cjdcordeiro cjdcordeiro self-assigned this May 16, 2024
Copy link

@linostar linostar left a comment

Choose a reason for hiding this comment

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

LGTM.

@cjdcordeiro cjdcordeiro merged commit 4051de3 into canonical:main May 22, 2024
6 checks passed
gregory-schiano pushed a commit to gregory-schiano/chisel-releases that referenced this pull request Jun 24, 2024
The maximum length of a line is now 100.
clay-lake pushed a commit to clay-lake/chisel-releases that referenced this pull request Nov 18, 2024
The maximum length of a line is now 100.
clay-lake pushed a commit to clay-lake/chisel-releases that referenced this pull request Nov 21, 2024
The maximum length of a line is now 100.
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

Successfully merging this pull request may close these issues.

4 participants