Consider removing the `lines_longer_than_80_chars` lint

dart-lang / ecosystem

This repository is home to general Dart Ecosystem tools and packages.

BSD 3-Clause "New" or "Revised" License

41 stars 6 forks source link

Consider removing the `lines_longer_than_80_chars` lint #280

Closed devoncarew closed 1 month ago

devoncarew commented 2 months ago

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 commented 2 months ago

cc @natebosch @matanlurey @Piinks @goderbauer

matanlurey commented 2 months ago

LGTM

natebosch commented 2 months ago

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 commented 2 months ago

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

devoncarew commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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?

https://github.com/dart-lang/pub/pull/4324 and https://github.com/dart-lang/pana/pull/1387 had a bunch of violations I believe.

natebosch commented 2 months ago

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.

https://github.com/dart-lang/pub/pull/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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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.