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

Consider removing the lines_longer_than_80_chars lint #280

Closed
devoncarew opened this issue Jul 17, 2024 · 11 comments
Closed

Consider removing the lines_longer_than_80_chars lint #280

devoncarew opened this issue Jul 17, 2024 · 11 comments

Comments

@devoncarew
Copy link
Member

We currently have lines_longer_than_80_chars in the lint set (under 'consistency'). We should consider removing it:

  • anecdotally, I've had to make changes to things like string constants to satisfy this lint that I haven't loved (breaking string constants in places I wouldn't otherwise want to)
  • all code should already be run on dartfmt; are the additional places this lint would catch (comments, string constants) worth the additional cost? does the lint pay its way in terms of benefits to the code vs effort to satisfy the lint?
@devoncarew
Copy link
Member Author

devoncarew commented Jul 18, 2024

@matanlurey
Copy link

LGTM

@natebosch
Copy link
Member

  • anecdotally, I've had to make changes to things like string constants to satisfy this lint that I haven't loved (breaking string constants in places I wouldn't otherwise want to)

What kinds of strings are they? I don't think I've hit cases where breaking a long string literal makes things less readable.

  • are the additional places this lint would catch (comments, string constants) worth the additional cost?

IME yes they are still worth it, because they otherwise come up in code reviews. If we decide to not comment on long lines as a policy too, I think we'd end up with very long lines showing up (especially in places like test case names).

@goderbauer
Copy link

I am in favor of removing this. This should be mostly covered by dartfmt.

@devoncarew
Copy link
Member Author

What kinds of strings are they? I don't think I've hit cases where breaking a long string literal makes things less readable.

Unfortunately these are things that I remember from PRs applying these lints but no longer have specific cases for.

@mosuem - I believe you said you had a project where this lint was firing - do you mind linking to a commit / diff for where it was triggering?

@devoncarew
Copy link
Member Author

devoncarew commented Jul 19, 2024

This CL turns on lines_longer_than_80_chars in the sdk tools/ dir as an example of general changes:

https://dart-review.googlesource.com/c/sdk/+/376701

The comment wrapping change is an improvement; several others seemed a bit awkward.

@mosuem
Copy link
Member

mosuem commented Jul 22, 2024

I believe you said you had a project where this lint was firing - do you mind linking to a commit / diff for where it was triggering?

dart-lang/pub#4324 and dart-lang/pana#1387 had a bunch of violations I believe.

@natebosch
Copy link
Member

natebosch commented Jul 22, 2024

Pub has 303 long line violations. I fixed the first 105 of them to get a feel for how it would be working with this lint in that repo.

dart-lang/pub#4332

I strongly suspect the pain is almost entirely going to be during the initial fix. None of these stand out to me as fixes that I'd be annoyed to have to make while I was authoring code day to day. Many of them were in strings that already were broken manually across lines, they just weren't broken at quite the 80 character boundary.

Quite a few of the fixes look like improvements to me. In particular, both here and in the SDK CL, the changes where a variable is extracted from a long complex string literal all seem to improve readability to me.

I'm in favor of allowing this an an exemption for projects which don't have the capacity to clean up existing violations, but I do think we get more value than pain out of including this in the team lint set.

@davidmorgan
Copy link

I don't particularly like this lint: it would be significantly improved if we had a quick fix to resplit strings and another to reflow comment blocks.

Most codebases I've worked in do require people to follow Effective Dart's guidance and do that manual work, and I think it's a win for readability overall. So if people aren't doing that then I think there's value to it.

@devoncarew
Copy link
Member Author

if we had a quick fix to ... to reflow comment blocks.

I would very much like this as well. I do wish this was part of dartfmt's mandate - that it would feel free to reflow comments. I'd love to be able to write a comment, not worry about manually wrapping the docs, and then have it all reflow when I hit save.

@devoncarew
Copy link
Member Author

As there isn't consensus here, and good arguments to keep, I'm going to go with 'no change'; we'll keep this rule in the rule set.

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

No branches or pull requests

6 participants