-
Notifications
You must be signed in to change notification settings - Fork 122
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
Comments
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:
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. |
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 |
You can pass |
I generally use Ctrl+Alt+L in IntelliJ IDEA, can't see an option in the plugin to pass this configuration :/ |
@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:
instead of lifting the 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. |
@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. :-) |
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. |
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. |
PSR-12 defines the concepts of soft limit and hard limit:
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. |
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. |
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. |
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:
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. |
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. |
@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. |
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. |
It appears that Linus Torvalds agrees with us: |
There are no plans to remove the option for specifying line length when running the formatter. |
I want auto formatter when saving. |
Linux uses 8 spaces of indentation, though, which Dart does not. |
This, to me, is the most frustrating pain point of Dart. We can run 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 |
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. |
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:
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. |
I work on a laptop, 1920x1080 so nothing fancy and I can side-by-side 100 or even longer files. |
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 |
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 |
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 |
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 |
5 years to get |
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. |
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 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 Likewise, libraries that generate Dart code and format it won't know to look for 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. |
Is there a reason to use a new(?) |
Oops, sorry, I meant We discussed a few options internally (
I realize the name is a bit funny because it's not really about "analysis", but that's fairly minor. |
just cross linking this (now closed) related issue #918 |
@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. |
I maintain an opinionated formatter. I always have many unpleased people. 🙃 But, ultimately we have to decide and I think |
Amazing! I have had hard conversations with developers who have changed the way they write code due to the line length limit. A layout that can be built via a single widget, aka Page A turns into
Widget1 Code architecture shouldn't be dictated by something as arbitrary and silly as line length. |
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. :) |
@munificent have you considered having a fallback on That's what
|
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 |
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:
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. |
…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.
I've closed this issue because the core work in dart_style is done now (🎉 ), but there's obviously work left to be done to integrate it into IDEs, code generators, etc. (And we have to ship the "tall-style" experiment.) I've filed this SDK issue to track that work: dart-lang/sdk#56863 |
Thanks @munificent, I understand now how this can be challenging and the effort you and the rest of the team is taking. Out of curiosity, can't you make the formatter accept a |
The library API doesn't take in analysis options as a string, but it does have a parameter for the page width.
Yeah, that's more or less how the code is organized. When I implemented it, I put all of the stuff related to analysis options in a separate set of libraries. Those libraries are decoupled from the rest of the formatter package and also talk to the file system that has an abstraction layer. The idea is that other tools that want to find, read, and parse analysis options files could then use those libraries. Right now, they're private inside dart_style because the formatter is the only package that we know needs to use it. Once we have another concrete use case, I plan to either leave those libraries inside dart_style but make them public, or move them to another more appropriate package (like maybe analyzer). Either way, yes, the goal is to prevent users from having to re-implement all of the logic to find, read, parse, and merge analysis options files. We're not quite there yet, but getting there shouldn't be too much work. I just didn't want to ship these as public APIs somewhere until I had at least one other integration to use as a test case to make sure the API is what we want. |
Awesome, I was just trying to see if there was a way to make the code generators implement this really easy, because from your previous message I understood this might not be simple for them to integrate, if ever possible. |
For code generators, there is an even simpler option. Put |
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:
I think this is perfectly valid for regular text for newspapers and magazines, that usually come in the form of:
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:
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.
The text was updated successfully, but these errors were encountered: