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

Rethink the 80 characters line limit #833

Closed
feinstein opened this issue Aug 5, 2019 · 81 comments · Fixed by #1571
Closed

Rethink the 80 characters line limit #833

feinstein opened this issue Aug 5, 2019 · 81 comments · Fixed by #1571

Comments

@feinstein
Copy link

feinstein commented Aug 5, 2019

I am not sure if this is the right repository to discuss this, but I would like to propose a investigation if 80 characters per line should still be the recommendation and dartfmt default.

The Dart style guide says:

Readability studies show that long lines of text are harder to read because your eye has to travel farther when moving to the beginning of the next line. This is why newspapers and magazines use multiple columns of text.

I think this is perfectly valid for regular text for newspapers and magazines, that usually come in the form of:

Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem
Ipsum has been the industry's standard dummy text ever since the 1500s, when an
unknown printer took a galley of type and scrambled it to make a type specimen book.

But recently with the soaring success of Flutter, we are finding deeply nested code, which leads to lots of space being wasted as pure indentation. This picture shows how one of my Flutter widgets has just about half its line wasted in indentation, forcing me to break lines all the time:

image

Bear in mind, this is just a 9 Widget deep tree:
Provider>Scaffold>SafeArea>WillPopScope>Column>Consumer>Padding>Row>Text.

And I didn't have any classes there with long names. If I did, it would have been a multi-lined mess.

IMHO, we can't use studies made for newspapers for coding styles, specially after the usage has become so different in the Flutter age.

I have seen people in the past defending 80 character lines saying it fits better all monitors, but nowadays with everyone using 24" monitors 1080+ I don't think that's longer the case.

I don't want to propose a new number, like 120 characters, but instead I would like to suggest that Google should launch a research on this, taking in consideration Flutter Apps and what line character count fits better their code.

My hope is that we don't turn out in the end with just 40 characters per line as real usable space due to indentations and enforced standards.

@munificent
Copy link
Member

we are finding deeply nested code, which leads to lots of space being wasted as pure indentation.

Increasing the column limit doesn't really fix that problem. You're still wasting just as much space on the left side of the page. You're just able to make the window wider and eat space on the right side of the page that you could be using for other things.

Some amount of nesting is, of course, natural in Flutter. But I don't think any page width is going to make >10 levels of indentation a good idea. That's just too much nesting for the eye to follow. Instead, I think a better solution is:

  • Language and API changes to get rid of unnecessary indentation. In particular, any widget that contains a list of child widgets ends up using two levels of indentation when really only one is needed.

  • Likewise, many widgets are conceptually more like modifiers than containers (think Center so I don't know if it's useful to burn a level of indentation on them as opposed to having them just adjacent to the widget they modify.

  • Teach users to break pieces of build() methods out into separate helper methods. This resets the nesting, makes them reusable (I see a lot of copy/paste code inside build() methods), and lets you attach a meaningful name to it.

  • Train users to be more proactive about breaking chunks of their build() methods out into separate widgets. Possibly investigate language or API changes to make this take less boilerplate to do.

I have seen people in the past defending 80 character lines saying it fits better all monitors, but nowadays with everyone using 24" monitors 1080+ I don't think that's longer the case.

I do all of my work on a 15" laptop and very much appreciate being able to do side-by-side diffs, so going wider than 80 columns would be a significant challenge for me.

@feinstein
Copy link
Author

The problem is when we are obligated to run dartfmt before each commit. this way we can't escape the laws of indentation, unless the Flutter team releases a new flutterfmt tool to help with this.

Also, breaking down into smaller Widgets is just swapping one problem for another. We reset the indentation, but now we have lots of boilerplate to code. Helper methods are in general not recommended . Also, sometimes splitting the Widget tree makes it easier to read when you care about the high level, but it also can make it harder, if you are trying to follow down the rabbit hole on where's the problem, and you find yourself going back and fort in the file, looking for different Widget classes, so it can be helpful or not, depends on the context.

I am not saying I am against breaking the build method, I do it all the time, it's just that sometimes it just doesn't make much sense, and if we use more specialized Widgets, like Provider and Consumer the tree will get even more nested and harder to find a logical place to isolate the code.

@munificent munificent transferred this issue from dart-lang/language Aug 5, 2019
@munificent
Copy link
Member

The problem is when we are obligated to run dartfmt before each commit.

You can pass --line-length=<value> to dartfmt to specify a custom column limit for whatever value you want. 80 is default, but this is one of the few places where it allows you to override that.

@feinstein
Copy link
Author

I generally use Ctrl+Alt+L in IntelliJ IDEA, can't see an option in the plugin to pass this configuration :/

@deakjahn
Copy link

deakjahn commented Aug 7, 2019

@feinstein I think that piece of advice about helper functions is about something entirely different. Not using mere functions instead of actual widgets. This has its clear reasons but that doesn't in any way forbid you from using helper methods inside your widgets. Basically:

@override
Widget build(BuildContext context) => InkWell(
      child: Stack(
        children: [
          _displayIcon(),
          _displayName(),
        ],
      ),
      onTap: () {
        if (onTap != null) onTap();
      },
    );

Widget _displayIcon() => CachedNetworkImage(
  // ...
    );

Widget _displayName() => Align(
  // ...
    );

instead of lifting the CachedNetworkImage and the Align (and everything that comes below it) up to the first function. I do it all the time for readability and it makes my life much easier.

Having said that, I wholeheartedly agree with the lifting of the 80 character limit which I have always found hilariously low in the age of 28-30-whatever monitors. And even on laptops.

@deakjahn
Copy link

deakjahn commented Aug 7, 2019

@feinstein by the way, if you go into Settings > Editor > Code style > Dart > Dartfmt, you'll see that it does pick up the width from another tab. Although I'm not sure it really does make any difference. :-)

@feinstein
Copy link
Author

I know that changing my code helps, but I feel this is like Apple saying "you are holding it wrong".

I just don't want to write additional code when I think my actual code is OK, just too much to the right. It's like creating a new problem for fixing an old one.

@greglittlefield-wf
Copy link

greglittlefield-wf commented May 4, 2020

As another data point, I did a search and found that ~35% of our organization's Dart projects (maintained by various independent teams) use a custom dartfmt line length of either 100 or 120 as opposed to the default 80.

And, almost all of those repos with custom line lengths contain over_react UI code, which has similar nesting/syntax to Flutter widgets.

@keomaborges
Copy link

PSR-12 defines the concepts of soft limit and hard limit:

There MUST NOT be a hard limit on line length.

The soft limit on line length MUST be 120 characters.

Lines SHOULD NOT be longer than 80 characters; lines longer than that SHOULD be split into multiple subsequent lines of no more than 80 characters each.

There MUST NOT be trailing whitespace at the end of lines.

Blank lines MAY be added to improve readability and to indicate related blocks of code except where explicitly forbidden.

There MUST NOT be more than one statement per line.

I think it is quite fair and looks nice for PHP. There might be something similar for Dart as well. In my opinion, 80 and 120 would be great.

@munificent
Copy link
Member

As another data point, I did a search and found that ~35% of our organization's Dart projects (maintained by various independent teams) use a custom dartfmt line length of either 100 or 120 as opposed to the default 80.

Sure, but another way to look at that data point is that ~65% of your teams are using the 80 character line length, which implies that it's the right choice to standardize on.

@ErliSoares
Copy link

As another data point, I did a search and found that ~35% of our organization's Dart projects (maintained by various independent teams) use a custom dartfmt line length of either 100 or 120 as opposed to the default 80.

Sure, but another way to look at that data point is that ~65% of your teams are using the 80 character line length, which implies that it's the right choice to standardize on.

They are using 80 characters per line does not mean that they think it is better, most programmers I know do not agree that 80 is better, but use it as a guideline.

Part of the projects is 80 characters per line and part 120, but if the guideline changes to 120 everyone would use the same pattern.

@deakjahn
Copy link

deakjahn commented May 29, 2020

No, Bob, it isn't. You can be as opinionated as you please in other corners of dartfmt, especially if, as right now, good placement of empty comments allows us to influence the formatting in many cases, but don't force line lengths on us, keep it a setting. Not everybody works in teams. Not everybody uses 14" laptop screens. Not everybody actually cares about old-fashioned ideas why the 80-limit should be better than anything larger of having no limit at all or whatever. It's really understandable that, for the sake of uniformity, you never wanted to introduce hundreds of user changeable settings. During the last year or so of working in Flutter I've grown to actually like the way dartfmt works (and in the few cases where I have differing opinion, I simple use those extra comments) but I'm sure to switch it off the minute you decide to rob the line length setting from us. I like working on a 30+" monitor and I want to use it all, not just the left quarter strip.

@vincevargadev
Copy link

There are advantages to embracing a universal line length limit in the community. It's the tabs-vs-spaces and other endless discussions happening all the time in all programming communities (e.g. Python's PEP8). This reminds me of the Go Proverb:

gofmt's style is no one's favorite, yet gofmt is everyone's favorite.

I don't really care if it's 80 or 100 characters, what I do not want is every package on pub.dev using different styling conventions and buggy, complex tools.

@feinstein
Copy link
Author

feinstein commented Jun 1, 2020

As I said in my original post, the 80 character line was made for newspapers and magazines, so eye movement be constrained to a region, this way reading would be more comfortable and easy to matain attention on the text.

But in Flutter we get lots of whitespace indentations on the left, making the text more like a zigzag pattern, not the same as a newspaper.

So here's a crazy idea to explore and research:

80 character lines counted after the first non-whitespace character in that line (left most whitespaces should be ignored on the character count).

This could be coupled with IDE tools that auto-scrolls the text left and right, keeping it bit centered as we scroll it up and down (but not perfectly centered, so we still get a sense of indentation as we scroll).

Is this good? Will it work? Will code be easier to read? Is this ridiculous and will it fail miserably? I have no idea, but I would love to see some research done on it, that's how science evolves afterall, crazy hypothesis being tested so we can come up with new ideas and new hypothesis based of the previous ones.

@deakjahn
Copy link

deakjahn commented Jun 1, 2020

@feinstein The 80-char has nothing to do with newspapers and magazines. That's an afterthought somebody tried to make up to justify that very old rule that simply goes back to the monitors of several decades ago.

@feinstein
Copy link
Author

Independently, my suggestion is based on the fact that Flutter code has lots of leading whitespaces, whereas normal texts does not, so my suggestion stands.

@feinstein
Copy link
Author

It appears that Linus Torvalds agrees with us:

https://www.theregister.com/2020/06/01/linux_5_7/

@munificent
Copy link
Member

don't force line lengths on us, keep it a setting.

There are no plans to remove the option for specifying line length when running the formatter.

@shinriyo
Copy link

I want auto formatter when saving.

@munificent
Copy link
Member

It appears that Linus Torvalds agrees with us:

https://www.theregister.com/2020/06/01/linux_5_7/

Linux uses 8 spaces of indentation, though, which Dart does not.

@lukepighetti
Copy link

lukepighetti commented Nov 16, 2020

This, to me, is the most frustrating pain point of Dart. We can run dartfmt with any line length we want, we can set up a single IDE to format with any line length we want, but we cannot enforce this project & IDE wide with a pubspec.yaml setting. I have no problem with https://pub.dev removing 20 points if someone doesn't use an 80 character line length. This isn't about public projects, this is about internal projects.

The only compelling argument I've heard against finalizing this feature is "because I don't want to," which honestly, is a fairly compelling argument in open source as far as I'm concerned, but it doesn't make it any less frustrating and senseless. I understand that maintaining dartfmt probably feels like taking on an army of slobbering screaming orcs with nothing but an xacto knife, but give the community a little bit of credit on this one @munificent .

@MrCsabaToth
Copy link

Even with 100 I can easily split the editor window vertically. 80 is so limited that it breaks even short+dumb code lines into multiple lines -> the brain has to work more cognitively to unwrap the riggidy broken lines.

MrCsabaToth added a commit to TrackMyIndoorWorkout/TrackMyIndoorWorkout that referenced this issue Feb 11, 2021
@azimuthdeveloper
Copy link

Just dug this up. I have a line length configured on my computer, and another one configured on my laptop. I have to remember, in my brain, the line length I have set, otherwise, I save files, and then it makes a git diff a total nightmare.

Also, this comment:

I do all of my work on a 15" laptop and very much appreciate being able to do side-by-side diffs, so going wider than 80 columns would be a significant challenge for me.

That's nice, and would relevant if you were the only person that used Flutter to develop apps. But you're not. So it's not relevant. And even if it was too wide or whatever, you could configure it by setting it in the pubspec.yaml as required.

@MrCsabaToth
Copy link

Just dug this up. I have a line length configured on my computer, and another one configured on my laptop. I have to remember, in my brain, the line length I have set, otherwise, I save files, and then it makes a git diff a total nightmare.

Also, this comment:

I do all of my work on a 15" laptop and very much appreciate being able to do side-by-side diffs, so going wider than 80 columns would be a significant challenge for me.

I work on a laptop, 1920x1080 so nothing fancy and I can side-by-side 100 or even longer files.

@Levi-Lesches
Copy link

I see there are two main arguments: 80 characters is standard, > 80 characters can look nicer. I think having the option for both should make everyone happy.

Since we can already override dart format, can there be another lint option too? Right now it's lines_longer_than_80_chars or nothing, but I agree that there should be some limit in a project. How about adding lines_longer_than_100_chars and lines_longer_than_120_chars as well?

@hbock-42
Copy link

Just give us a config file where we can set line length per project, nothing else ...
This is ridiculous, I am working solo on the mobile part, but will have to onboard new members soon. I praised dart and flutter, easy configuration project wise (linter file), but I forget the line length is per user.
I tried to convert back to 80 line length just to see how it renders, and this is seriously ridiculous, even on a laptop ...
I mean I am not coding on a tablet...

@Dynesshely
Copy link

Just give us a config file where we can set line length per project, nothing else ... This is ridiculous, I am working solo on the mobile part, but will have to onboard new members soon. I praised dart and flutter, easy configuration project wise (linter file), but I forget the line length is per user. I tried to convert back to 80 line length just to see how it renders, and this is seriously ridiculous, even on a laptop ... I mean I am not coding on a tablet...

I totally agree with this comment

We can consider setting the maximum length of each line in the lint configuration file
Avoid using dart format's default maximum length per line
Similarly, this item should also be followed in the score of pub.dev to detect
Different projects may have different maximum length settings for each line
If this configuration cannot be made to depend on the project, it will not be friendly to distributed collaborative development projects.

Consider an open source project. New contributors may not necessarily read the contributing file in detail, or there may be no such file at all. If these automatic formatting settings can be determined by the lint configuration file, and vscode or plug-ins on other platforms can also Following these settings, it will become very user-friendly, and developers only need to focus on the code logic.

@hbock-42
Copy link

I think at this point it's just that they want to show us THEY get to choose those stuff, there is zero good reason not to add this feature but they don't want to admit they are wrong so they will just prevent us from having this basic and very needed functionality

@deakjahn
Copy link

deakjahn commented Apr 20, 2024

Dart format was opinionated, always, and Munificent (Bob) was always vocal about it. It usually grows on you, the more you use it. Only that I always thought that it could be opinionated inside the constraints but the line length is such a constraint that should be removed from format and dictated from the outside.

@jamesderlin
Copy link
Contributor

I think at this point it's just that they want to show us THEY get to choose those stuff...

Let's try to keep the discussion constructive and to not be overly dramatic. If your claim were true there wouldn't be a way to configure line length at all.

@Dynesshely
Copy link

I think at this point it's just that they want to show us THEY get to choose those stuff...

Let's try to keep the discussion constructive and to not be overly dramatic. If your claim were true there wouldn't be a way to configure line length at all.

How about put the configuration in analysis_options.yml ?
If no this file or no this configuration in this file, we keep 80 per line ?

@y-nk
Copy link

y-nk commented Jul 5, 2024

5 years to get dart format to respect .editorconfig and still nothing. i wonder how you can advocate to build such as flutter (which builds apps for end users) when you don't listen to your own users. there's one level of stubborn-ness, but this is something else. i'm sure people of this thread complaining remain on dart by constraint rather than by choice, and if anything will kickstart a non-dart project as soon as they can (to what repo owners may answer: "yeah cool thx bye")

@deakjahn
Copy link

deakjahn commented Jul 5, 2024

No, you're not right. I use Flutter (and Dart) because of my own choice, because I consider it the best available cross-platform framework, and not just by one long sea mile. I had my fair share of criticism of this 80-char limit, and still. Format always was opinionated, from the very start, Munificent (its author) was always vocal about this. I have no idea, either, how we could convince him that as correct as he can be on other topics, he isn't on this one, but that doesn't make me abandon ship. Very, very, very far from it.

@munificent
Copy link
Member

munificent commented Oct 4, 2024

OK, we're going to try to support project-wide configuration of page width.

I have a PR out which does the first piece of this: looking for a surrounding analysis_options.yaml file when you run dart format. Once that lands (or before I guess if you want to check out the branch), you are all welcome to try it out and provide feedback on the UX.

I would caution against using this seriously just yet. One of the main reasons we have been so hesitant to support project-wide configuration is that it's really easy to break all sorts of tools and workflows if we aren't careful.

In particular, analysis_server and other IDE integrations that use the formatter as a library don't know to look for this configured page width (yet). That means you'll get a different formatting width in your editor than when you run dart format. I'll be working with various teams that maintain tools that use the formatter as a library to also look for analysis_options.yaml configuration and pass in the appropriate page width. That will take time.

Likewise, libraries that generate Dart code and format it won't know to look for analysis_options.yaml files to format at the width of the project that they are generating code into. For generators like the ones used in build_runner, they can't access that configuration file if they wanted to.

For that, we're planning to also support comments inside a file to indicate the formatted width. Code generators would be updated to include that comment in any code they generate so that the code's formatting is then independent of any surrounding configuration.

Until all of this is done, it's very likely that you'll break any CI system that is checking that your code is formatted, has a project-wide width set, and is using code generation.

Getting all this sorted out is surprisingly hard. For many years, one of the really nice things about the formatter's model is that formatting is essentially a stateless pure function: you pass in a string and get a string back. That's true regardless of how or where you run the formatter. String in, string out.

With this change, that's no longer true. The formatted output now depends on the state of the surrounding file system and whether the code talking to the formatter knows to plumb the page width in correctly. Getting every piece of code that uses the formatter to understand that new world is going to be a lot of work and take time to shake out the bugs.

It would be simpler and less brittle and buggy for all of us if we just stuck with a canonical width, but I understand that this is an important issue for many users so I think it's worth supporting it and pushing through all of the changes required to get everything working right.

@jamesderlin
Copy link
Contributor

Is there a reason to use a new(?) package_config.yaml file instead of .editorconfig, which a number of editors already support?

@munificent
Copy link
Member

munificent commented Oct 4, 2024

Oops, sorry, I meant analysis_options.yaml. No new files. :)

We discussed a few options internally (pubspec.yaml, .dart_tool/package_config.json, .editorconfig, and analysis_options.yaml). They all have their various pros and cons but I think analysis_options.yaml is the best fit:

  1. Analyzer and some other tools are already looking for and parsing it.
  2. There is a line rule for checking line length. That rule will need to be updated to take the configured length into account. Linter rules are already configured using analysis_options.yaml.
  3. The analysis_options.yaml file supports include so that you can reuse configurations across multiple packages and projects, like most users do with the recommended lint set. The page width configuration works with that, so an organization could have a canonical analysis_options.yaml file that is used by all of their packages.
  4. Conversely, you can have a separate analysis_options.yaml file for a single subdirectory, so you can have configuration more fine-grained than package-level if you want. Or put it in a directory above a bunch of packages and it will configure all of them.

I realize the name is a bit funny because it's not really about "analysis", but that's fairly minor.

@lukepighetti
Copy link

lukepighetti commented Oct 4, 2024

just cross linking this (now closed) related issue #918

@gsouf
Copy link

gsouf commented Oct 4, 2024

@munificent my guess is that .editorconfig is such a standard nowadays that you're going to have many unpleased people if line length specified in .editorconfig is not respected.

@munificent
Copy link
Member

munificent commented Oct 4, 2024

you're going to have many unpleased people

I maintain an opinionated formatter. I always have many unpleased people. 🙃

But, ultimately we have to decide and I think analysis_options.yaml works better with the rest of the Dart ecosystem. It's a single integer, so I think users will be able to manage putting it in both analysis_options.yaml for Dart and .editorconfig for other languages.

@rayliverified
Copy link

rayliverified commented Oct 4, 2024

Amazing! I have had hard conversations with developers who have changed the way they write code due to the line length limit.
Because the widgets don't fit, they break them into separate widgets needlessly.

A layout that can be built via a single widget, aka Page A turns into
Page A

  • Widget1
  • Widget2
  • Widget3
  • Widget4

Widget1
Widget2
Widget3
Widget4

Code architecture shouldn't be dictated by something as arbitrary and silly as line length.

@munificent
Copy link
Member

Because the widgets don't fit, they break them into separate widgets needlessly.

For what it's worth, one of the reasons I prefer the 80 column line length is because it encourages people to break their code into smaller more easily readable units like this. Often, when I see code that feels cramped at 80 columns, it's because a single statement is doing too much work to be easy to read and would be better split into a couple of local variables.

Maybe I just have too small of a mind and can't handle code that's too wide to fit in it. :)

@gsouf
Copy link

gsouf commented Oct 4, 2024

But, ultimately we have to decide and I think analysis_options.yaml works better with the rest of the Dart ecosystem. It's a single integer, so I think users will be able to manage putting it in both analysis_options.yaml for Dart and .editorconfig for other languages.

@munificent have you considered having a fallback on .editorconfig if the value is not specified in analysis_options ?

That's what prettier from the js ecosystem does: https://prettier.io/docs/en/configuration.html#editorconfig

If a .editorconfig file is in your project, Prettier will parse it and convert its properties to the corresponding Prettier configuration. This configuration will be overridden by .prettierrc, etc.

@deakjahn
Copy link

deakjahn commented Oct 4, 2024

For what it's worth, one of the reasons I prefer the 80 column line length is because it encourages people to break their code into smaller more easily readable units like this

As one of the long time vocal supporters of the non-80 scenario :-), using a large monitor, I also break them up. Not necessarily into separate widgets but separate buildXXX() functions, all the time, I never really have a monolithic upper level build(). But this has much more to do with vertical space than horizontal: even if I have plenty horizontally, I will run out of lines vertically so it pays off to have something that I can have an overview of. So, I don't think this is really that much related.

@munificent
Copy link
Member

munificent commented Oct 4, 2024

@munificent have you considered having a fallback on .editorconfig if the value is not specified in analysis_options ?

I'd like to keep things as simple as possible. Dealing with the file system tends to be a bug farm with lots of rare, weird failure modes. Every additional file we look for and use multiplies those failure modes. Some examples off the top of my head:

  • If we find an analysis_options.yaml file but it doesn't have a formatter: key, do we keep looking for a .editorconfig or do we treat "yes options file but not key" to mean an explicit configuration of the page width?
  • What if the analysis_options.yaml file has formatter: but no page_width: under it? What if page_width is there but isn't an int?
  • If we find a .editorconfig in a subdirectory, but there is an analysis_options.yaml file in an outer directory, which one wins? What if their locations are reversed? What if they're in the same directory?

Etc., etc. All of those combinations have to be figured out and tested. Also, every package that uses the formatter as a library needs to support all of this same logic or you'll get different formatted output in different IDEs/editors/etc.

I think it's easiest to just put the value in one canonical place.

munificent added a commit that referenced this issue Oct 7, 2024
…s_options.yaml file (#1571)

When using the dart format CLI (and not the library API), if the user doesn't specify a page width using --line-length, then the formatter will walk the directories surrounding each formatted file looking for an analysis_options.yaml file. If one is found, then it looks for a configured page width like:

```
formatter:
  page_width: 123
```

If found, then the file is formatted at that page width. If any sort of failure occurs, the default page width is used instead.

This is hidden behind the "tall-style" experiment flag and the intent is to ship this when the rest of the new tall style ships.

This is a fairly large change. To try to make it easier to review, I broke it into a series of hopefully more digestible commits. You might want to review those separately.

Fix #833.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet