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

allow configuration of line length in pubspec.yaml #918

Closed
lukepighetti opened this issue Apr 15, 2020 · 102 comments
Closed

allow configuration of line length in pubspec.yaml #918

lukepighetti opened this issue Apr 15, 2020 · 102 comments

Comments

@lukepighetti
Copy link

lukepighetti commented Apr 15, 2020

Hey @munificent is it possible to allow configuration of dartfmt line length in pubspec.yaml or some other common project configuration file? Of the last 5 flutter packages I've pulled down to work on 2 were using a 120 character line length which is becoming more and more common in Flutter projects.

I understand that we have access to IDE settings for line length but this is non-viable because it's IDE specific and not project-specific. Project specific would allow a package maintainer to make an increasingly common style choice and have all maintainers automatically use it without having to worry about IDE settings.

Any thoughts?

name: foo
description: An example app.
version: 1.0.0

environment:
  sdk: ">=2.6.0 <3.0.0"

dartfmt:
  line_length: 120
@munificent
Copy link
Member

Hi, Luke. I think you can probably guess what my thoughts are: https://github.com/dart-lang/dart_style/wiki/FAQ#why-cant-i-configure-it :)

If I were to add support for this, the very next day, all progress across the entire Dart ecosystem would screech to a halt as team members spent all day debating what line length should go into their package's config. I have seen no data to support the idea that programs vary such in their form such that it is measurably better to have different line lengths for different programs.

I took a corpus of the 994 most recent pub packages as of January. Of them, 763 contain ".dart" files with at least one line longer than 80 characters. When I exclude lines that start with "import", "export", or "part" that drops to 756. Skimming the result, most of long lines seem to be doc comments or long string literals, both of which aren't split by dartfmt.

If I exclude lines containing "//" or a quote character, then only 335 (34%) packages contain any files with a line over 80 characters.

I went ahead and looked at the individual lines of code in every file. Ignoring any trivial line that contains only whitespace or a punctuation character or two, I can bucket the remaining lines into "short" (<= 80 chars) and "long" (> 80).

212 (21%) packages have a ratio of 1% or less long lines to short. Only 62 (6%) packages have more than 5% of their non-trivial lines over 80 characters.

This says to me that most users are comfortable fitting within the current line length. And, even in packages that do go over 80 characters, it seems to rarely actually be helpful. Even packages that do have long lines, indicating that the user didn't format or formatted with a longer length have very few lines that actually exceed 80. That means the wider page size isn't buying them much except for wasted screen real estate on the majority of lines that don't benefit.

@lukepighetti
Copy link
Author

lukepighetti commented Apr 21, 2020

I can see that you've spent a lot of time thinking about this. The problem with your stance is that projects already do make the decision whether to use the default line length or a custom one, the problem is that when they make that decision they aren't able to enforce it project wide. So not allowing line length settings in pubspec actually degrades the consistency of dartfmt in practice compared to leaving it up to IDE config files.

You even state that line length can be handled by the user, yet we still cannot set this project level.

#833 (comment)

@munificent
Copy link
Member

projects already do make the decision whether to use the default line length or a custom one

Some do, but most don't.

the problem is that when they make that decision they aren't able to enforce it project wide.

Yes, that is a valid problem for those teams. But part of maintaining an open source project like dartfmt is deciding which use cases to support and which to not. Time is finite and time spent adding this feature is time not spent doing other work that benefits other users.

In this particular case, my belief is that the feature has net negative value across the entire ecosystem. I have yet to see any evidence that a line length longer than 80 columns actually improves productivity or readability to any measurable degree. It is easy to demonstrate that going wider than 80 columns makes it harder to do side-by-side diffs or two-panel editing on laptop screens (which is many of us right now and even in the best of times, many users cannot afford large displays). We can directly observe the the time spent arguing over style choices in code reviews and elsewhere when users are given the ability to make style choices.

I understand that this particular issue is very close to your heart. But my very clear experience over several years of maintaining dartfmt, personally interacting with hundreds of users, and seeing dartfmt enforced across millions and millions of lines of code is that the best experience for all users is to just let it do its job, move on with your life, and focus on what your code does and not how it looks.

@ajay-mm
Copy link

ajay-mm commented Oct 6, 2020

@munificent Making dartfmt use line length from pubspec does not in any way create any issues for those developers with 80 char width devices. It is an easier way of enforcing 80 character limit for projects that want to enforce it, instead of asking every developer to change their ide settings or add the command line argument while formatting.

You can keep your side by side diffs for projects you work on. But don't enforce your constraints on other people. @lukepighetti isn't asking you to change the defaults. He is only asking for a very simple, straight and sensible way to do something which is already possible today but requires lot of manual efforts for everyone involved.

@lukepighetti
Copy link
Author

lukepighetti commented Oct 7, 2020

As of today line length is user configurable but its fractured and unreliable because it can only be set in IDE configurations (like .vscode/settings.json) but it cannot be set in pubspec.yaml. If we could set it project-wide in pubspec.yaml then any user of any IDE would be able to minimize diffs when working on a project.

Also, the 80 character line limit is an arbitrary and unfair constraint on Flutter developers. A growing number of Flutter projects are switching to 120 character line limits in order to keep using dartfmt and get more pleasing results with deeper trees and longer class declarations with common mixins.

I have no problem if pub.dev wants to hammer scores if packages aren't 80 character line width, but for private projects it seems reasonable to fully implement this feature. If the 80 character width is so important, get rid of the ability to run dartfmt with a different line length.

@israellias
Copy link

+1

@Antoniossss
Copy link

Antoniossss commented Dec 11, 2020

AVOID lines longer than 80 characters

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.

But programming is NOT a newspaper. If 80 chars line lenght is that great, why people keep asking to change that from pubspect (and in the meantime changing that per IDE instance)?
Studies done on (newspaper) readers are applied to programers. I personally sometimes feel that 160 is too small, but thats what I am using for most of my projects.

+1, allow to change that and lower the "readibility" score.

@crimsonvspurple
Copy link

Please allow line length setting to be set in pubspec as not having this causes problems for anyone no wanting to use default 80 chars.

With ultrawides being more common and descriptive class/variable names, 80 chars limit is too low. We use 160 for all our projects.

@flukejones
Copy link

This is becoming very frustrating to deal with. We no-longer use 4:3 800x600 displays, and have 16:9, 16:10, or even 21:9 displays at high res which can comfortably use higher that 80 char lines, in multiple views.

80 char line limits become even more of a problem when it unnecessarily breaks a line that is fine at say 100 char because the variable names are perhaps a little longer for descriptiveness (e.r, status.isPermanentlyDenied). Such instances reduce readability quite drastically.

80 char line limit enforcement across a whole ecosystem is frankly baffling. And archaic.

@flukejones
Copy link

flukejones commented Aug 9, 2021

It's as simple as this:

80 char limit:

        builder:
            (BuildContext context, Box<MdlMachine> machines, Widget? child) {
          return OrientationBuilder(
              builder: (BuildContext context, Orientation ori) {
            var nameInBody = false;

100 char limit:

        builder: (BuildContext context, Box<MdlMachine> machines, Widget? child) {
          return OrientationBuilder(builder: (BuildContext context, Orientation ori) {
            var nameInBody = false;

I gain nothing from being forced in to 80 char limits here except for wasted lines. And the further you get in to flutter the worse it can get, particularly since widget properties can become verbose and are often required to be descriptive to convey their meanings and requirements easily.

I see vscode plugin at least has "dart.lineLength": 100,. But this really must be in the pubspec.yaml

@munificent
Copy link
Member

munificent commented Aug 10, 2021

Please allow line length setting to be set in pubspec as not having this causes problems for anyone no wanting to use default 80 chars.

dartfmt does not have any awareness of pubspecs. Right now, it just formats each file individually completely context-free. This makes it easy and fast to run it in a variety of configurations. For example, many text editors pipe source to it through stdin. Changing dartfmt to scan parent directories for pubspecs would be a significant change in how it behaves.

80 char limit:

        builder:
            (BuildContext context, Box<MdlMachine> machines, Widget? child) {
          return OrientationBuilder(
              builder: (BuildContext context, Orientation ori) {
            var nameInBody = false;

100 char limit:

        builder: (BuildContext context, Box<MdlMachine> machines, Widget? child) {
          return OrientationBuilder(builder: (BuildContext context, Orientation ori) {
            var nameInBody = false;

Following the recommended style would give you:

        builder: (context, machines, child) {
          return OrientationBuilder(builder: (context, ori) {
            var nameInBody = false;

Ultimately, it's your code to style and format as you please. dartfmt is happy to format longer line lengths if you tell it to, and you certainly don't have to follow "Effective Dart". Our tools default to the style they do because we believe it's the most productive for the most users.

Is that default going to please all users? No. No default would. I've had users say they'll never use Dart again if we get rid of semicolons and others say that will never use Dart until it gets rid of them. You can't please everyone. But having nice defaults that most people agree on makes it easier for all of us to read each others' code, even if it's not in our personal preferred style.

@crimsonvspurple
Copy link

crimsonvspurple commented Oct 2, 2021

Default can be 80 or 60 or 100. That's is fine. I don't care about that.

I believe the reason people are posting here about it, is: You set longer line lengths. Commit push codes. Some day some other developer pulls, edits code, runs a format and things go back to default line length.

Basically, I (and probably many others) are asking for a way to have the line length property within the project itself so that regardless of who opens it wherever, code formatting stays the same. The default value or changing the default value in dart style guide is not related to this.

I have personally worked around it by committing select IDE project files in git for now.

@lukepighetti
Copy link
Author

lukepighetti commented Nov 22, 2021

Just had a situation where someone submitted a PR to my repo with their own line length. If line length were project specific (defined in pubspec.yaml) and not left to IDE settings, I believe this could have been avoided.

I am faced with a situation where I feel compelled to create IDE specific configs in all my dart projects to try to enforce the 80 character default because people appear to be resorting to global IDE settings since there isn't a convention built around handling this at the project level in pubspec.yaml

lukepighetti/salomon_bottom_bar#15

@lukepighetti
Copy link
Author

lukepighetti commented Nov 22, 2021

Also, had a chat with a Kotlin dev the other day who was lamenting that we couldn't have Dart on the same line length as our Kotlin and Swift codebases because there was no way to configure it at the project level (in this case, 120 characters wide)

@lukepighetti
Copy link
Author

Ran into this again today, this repo is using >80 char line length. I have no problems reading and manipulating the code. But because there is no pubspec.yaml setting for line length, my IDE is breaking formatting and increasing diffs unnecessarily. https://github.com/osaxma/dfs

@munificent
Copy link
Member

Copying my comment from here:

Dart format does not have any notion of "project" or "package" and doesn't know where to look for any sort of global or package-level configuration data. It just formats each file path you give in a completely context-free manner.

This is important because:

  • It's faster. The formatter doesn't look through parent directories for config files and determine which package a given file belongs to before it can determine how to format it.
  • It ensures you get consistent formatting when formatting files on disk or in your editor. A lot of editors invoke dart format by passing a string of code to it on stdin and reading the result back from stdout. For those invocations, there is no file path that the formatter could use to find the config file. We could require IDEs to pass in the desired line length, but that's likely to be brittle.

We have millions and millons of lines of Dart code inside Google formatted to 80 columns. I can absolutely promise you that Googlers have come up with class names and method names as long as or longer than anything you see in the wild. This is nowhere near the top of the priority list for improving Googlers' lives and I find it very hard to believe that configurable line length will measurably improve the lives of any external users either.

I have never seen code that I felt would be easier to read with a longer line length. I've seen a lot of code that would be easier to read if the author rethought the verbosity of some of their identifiers.

@esDotDev
Copy link

esDotDev commented Jan 12, 2022

The problem with your stance is that projects already do make the decision whether to use the default line length or a custom one, the problem is that when they make that decision they aren't able to enforce it project wide.

Exactly, I haven't been on a Flutter team that hasn't already had this debate. It usually lasts about 20 minutes, and we settle on 100 or 120.

This debate is already being had, we just don't have a way to enforce it. Which is much more of a productivity blocker than the initial debate, as each time a new team member is onboarded to a project, they start screwing up the git diffs.

@munificent
Copy link
Member

This debate is already being had,

This is true for some teams, but not all. If we add support for configuring it the formatter, than we will expand the set of teams that end up having this debate. That's an anti-goal for the formatter. One of the key reasons this software exists at all is to reduce the set of things teams debate.

Imagine that someone passed a law—literally a criminal offense—if you formatted your code longer than 80 columns. You were forced to comply. Do you honestly believe that your engineering productivity or overall quality of life as a human on Earth would be measurably negatively impacted? If not... why not just stick with 80 and spend your limited time on this spinning planet on stuff that matters?

@lukepighetti
Copy link
Author

My stance is that configurable line length is half implemented. We should either remove the ability to format at custom line lengths entirely, or complete the implementation and allow it to be set at the project level

@esDotDev
Copy link

esDotDev commented Jan 12, 2022

Do you honestly believe that your engineering productivity or overall quality of life as a human on Earth would be measurably negatively impacted? If not... why not just stick with 80 and spend your limited time on this spinning planet on stuff that matters?

Measurably is the key word here, but yes, my day to day enjoyment on my ultrawide monitor is negatively impacted substantially when reading and writing Flutter code that needs to break at 80 chars.

Not so much package code, because package code tends to be less indented overall. But Flutter day-to-day UI code? It is quite annoying yes.

The reason all of my package code is still submitted at 120lines, is because I'm not about to start changing my IDE settings everytime I jump back and forth between projects and packages, which I would inevitably forget, and the results of that are not fun.

Could I live with it? Of course, but I don't find that a very convincing argument. This is a discussion about quality of life discussion where small things matter.

we will expand the set of teams that end up having this debate.

True, but you miss a key benefit in your overview here. For the teams that do have this debate, you will enhance their productivity because they can properly implement the results of their debate and be done with it.

@munificent
Copy link
Member

munificent commented Jan 12, 2022

We should either remove the ability to format at custom line lengths entirely, or complete the implementation and allow it to be set at the project level

Many many many command line tools accept options on the command line that aren't also available from an implicitly discovered configuration file, so I don't see accepting the line-length argument as being an incomplete implementation.

For the teams that do have this debate, you will enhance their productivity because they can properly implement the results of their debate and be done with it.

At the expense of lowering the productivity of every user that switches between multiple codebases that would configure their line lengths differently. Every time you clicked "go to definition" on some imported library and ended up in a file whose line length doesn't fit in your IDE window, you'd have a bad experience.

The charter of dart_style is to help the Dart ecosystem, not just to improve the lives of individual developers. It is a public park, not a lawnmower you can use for your lawn. All are welcome to visit its park and because it is a shared park used by many, it can be particularly well manicured. But visiting it means accepting that maybe the grass is not cut to the exact length you might personally prefer. Hopefully, being able to explore an entire park (the entire package ecosystem) more easily makes that trade-off worth it.

@lukepighetti
Copy link
Author

Many many many command line tools accept options on the command line that aren't also available from an implicitly discovered configuration file, so I don't see accepting the line-length argument as being an incomplete implementation.

Looking at the context of code formatters, dartfmt is the only one I'm aware of that has a line-length argument but no way to set it for the project outside of IDE config files. https://prettier.io/docs/en/options.html#print-width

@flukejones
Copy link

An advanced search of github for "max_width =" language:TOML reveals line widths from 80 up to 160, with the most common being 100, for the rustfmt.toml that rust uses. And somehow this absolutely does not affect the Rust ecosystem.

None of the arguments made to keep 80 column limit as a hard limit make sense and it just inconveniences the owners of the repos. The "But googlers don't need it" argument is not as good as you think - are you going to tell us to write shorter function and variable names to compensate for the forced line length also?

It is odd that a language as configurable as Dart, with a ridiculous variety of styles to write in, such that packages like "pedantic" are a thing, is getting so hung up on a single thing - line length. Change any one of these options but don't you dare go over 80 char wide for lines.

@esDotDev
Copy link

esDotDev commented Jan 12, 2022

At the expense of lowering the productivity of every user that switches between multiple codebases that would configure their line lengths differently. Every time you clicked "go to definition" on some imported library and ended up in a file whose line length doesn't fit in your IDE window, you'd have a bad experience.

No, not every user. Just users on very small screens, like 16" laptops. I don't know why we would optimize for the hobbyist developer that does not have a 2nd monitor attached. And if we are, why stop there, if you set it to 40 you could even include phone users. The obvious answer is that by going too short, you negatively impact users on large monitors, which is what we are complaining about here. It seems pretty arbitrary, especially in a world where programmers bill out at $5000/wk, but a larger monitor to make you more efficient, is $200.

visiting it means accepting that maybe the grass is not cut to the exact length you might personally prefer.

I take your point about consistency, but the analogy feels more like a park that is ostensibly build for adults, but all the rides are made for people 4ft tall :p

Maybe I'm being to cynical, but it really does feel like the underlying cause here is most of the googlers use laptops as their primary coding environment, so it's optimized for small screens, and all of this other stuff is just rationalizing reasons for that optimization.

But there's no point in debating the default, as it is so highly contextual. We should be able to set it on a per project basis though, if that is our wish, and in a way that does incur technical debt if we really do care about overall developer productivity.

@munificent
Copy link
Member

The "But googlers don't need it" argument is not as good as you think - are you going to tell us to write shorter function and variable names to compensate for the forced line length also?

What I am suggesting is that your code would be more readable to all who read it if you considered shorter function and variable names and hoisting more code out of deeply nested subexpressions into local variables or helper functions. Using a wider line length is treating the symptom.

I have seen a lot of code that is clearly improved by the author rethinking names and nesting structure when they find it line wraps too often. I've never seen code whose readability I felt was improved by making the lines wider. Just because you have a bigger screen, that doesn't mean you have bigger eyes or a bigger brain. Long identifiers and deep expressions still take more effort to read, regardless of how they line wrap.

If you prefer particularly long identifiers and deep expressions, that's your choice. It's your code. But I don't think it benefits the larger Dart ecosystem to tailor the tooling around that choice. And I've never been convinced that it's a good use of the time that Google pays me for to implement support for configuring that in dart_style when I could be spending that time on other things.

@flukejones
Copy link

@munificent that is of course very good advice and I don't doubt for a second that all of us here follow it.

You keep framing this as "googlers" and "ecosystem" though.

Googlers seem to have a different hardware setup that maybe justifies this choice - I don't, and neither do my work colleagues (we use ultrawides). We have a 60,000 line codebase which will never see the light of day outside our offices so "ecosystem" is not a concern for us.

Depending on the options you select for dart-analysis which is ridiculously configurable, this affects heavily how you write your code and has more impact on it than what may have been a previously okay-length var/func name.

For example setting unnecessary_new and unnecessary_this has an impact on line length. Same with always_declare_return_types and especially always_specify_types. Yet none of these are enforced, and depending on the rule-set package you use it affects how long your lines are. If you say " isn't recommended" then why is it an option to start with?

Allowing the IDE like to pass a line-length to dartfmt, but not saving that length in a config for the project is confusing and risks a project ending up with odd formatting.

The enforced line length is completely at odds with the amount of configuration dart-analysis allows - and as rust has shown, it doesn't impact the ecosystem in the slightest.

@rydmike
Copy link

rydmike commented Jan 14, 2022

A "fairly" known Dart repo that uses lines much longer than 80 chars and don't even use dartfmt is of course the Flutter repo itself.

Which is kind of ironic since dart and flutter packages on pub get a score penalty if dartfmt and max 80 char line length is not used.

I think both @lukepighetti and @esDotDev made excellent points on why being able to set the line length on a project level for dartfmt would be very useful.

I would definitely prefer to use eg 120 on a project level too, because we use the lint that the Flutter repo does, the one that requires you to specify all types. This extra, but in our opinion nice formal verbosity, definitely adds to line lengths and causes more not so terrific looking line breaks. It would be nicer at 120, but we have stuck to 80 due to the fact that it cannot be set on project level.

@esDotDev
Copy link

esDotDev commented Jan 14, 2022

The truth is, leaving teams in a state of technical debt because they can not properly specifiy a team-wide setting like line-length, is an order of magnitude more disruptive than having a debate about line-length in the first place.

And as flukejones mentioned, line length is one of a dozen different discussions you need to have when discussing lint rules in dart for any project. (And usually one of the shorter, easier discussions, tbh) It's awesome that we can define all of these options as our team desires, but then bizarre that we are blocked on setting something as intrinsic to our workflows and production as preferred max line length. Not being able to set it leads directly to time-consuming issues with version control.

If the argument is truly about developer efficiency, there is no debate imo, this policy does far more harm than good.

I think the fact that the Flutter team opted not to use 80 char breaks flies pretty squarely in the face of "most people have no problem with it", which is not really a good argument anyways, when dart is so otherwise customizable, but even if it were a good argument, it doesn't seem to be true.

When combining: "always specify types" with Flutters "everything is a widget" philosophy, which means a lot of indentation, is it really surprising that 80chars is not working for us, when it works fine for pure dart devs?

@munificent
Copy link
Member

I'm having a hard time understanding if the team would accept a PR that fit my proposal above, along with addressing the concerns from Jake offered here.

This isn't a single-PR-scale change. Implementing this only in dart_style would make the command line dart format behave differently from the IDEs almost everyone is using. Most IDE integrations format files that are in memory, not on disk. They use the formatter's library API to do that, and in that mode, the formatter should probably not be wandering around the user's filesystem trying to find a config file. The config itself may have an unsaved change to the line length which should be honored when formatting happens in an IDE.

In fact, the dart_style library can't look at the file system without it being a major breaking API change. Currently, the dart_style library doesn't depend on dart:io, which allows it to be used on the web, Flutter, or other platforms or contexts where dart:io isn't available.

To add support for this, we would have to coordinate the design with at least the IntelliJ and VSCode plug-in folks. They would need to add support for finding this config file through their own file system abstraction layer and sending the correct line length for each file when using the formatter's library API. I don't know if they have the bandwidth for that. Other editors with less active support may end up broken or inconsistent until/if they are updated too.

There are design questions around the configuration itself:

  • Does the configuration go in the pubspec? build.yaml? analysis_options.yaml? Somewhere else? What is its schema? How are configuration errors handled?

  • What happens if there are configs at multiple levels of the directory hierarchy? Does the one nearest to each file being formatted win? Or does the config in the same directory where the formatter is invoked win?

  • How do code generators that use the formatter to format their results know what line length configuration to use? Who will own updating those generators to determine and pass in the right line length?

  • What happens when file IO or permission errors occur when looking up the directory hierarchy to find the config file? (These do indeed happen.) What if those errors are intermittent?

    (Note that it's not enough for the implementation to handle this. The PR would have to include tests of it. Writing file IO tests is time-consuming, annoying, and really hard to get working across all platforms. But if we don't have those tests, then it's too easy to regress that behavior.)

  • When looking for a config file, does it stop walking the directory hierarchy at some point and give up? (For example, Black stops when it sees a .git or .hg directory.)

  • What happens if the user passes a --line-length argument and there's a config file? Does that command line option specify a default or an override?

  • What happens when users want to configure the line length differently for different parts of the same package? Is there a mechanism for this? Do we end up also needing per-file configuration?

  • If we find a config file that doesn't configure the line length, should we keep walking up the directory hierarchy looking for a surrounding one that does? Is the absence of a line length configuration treated as "configure this directory's files to the default" or "use any surrounding configuration"?

  • Assuming this goes in the pubspec, is there a way to configure the line length for a directory without creating a pubspec? Should there be?

I'm sure all of these are tractable, but are decisions that would have to be made. I'm sure there are other design questions I'm not thinking of.

If this was something I could just hack together in a couple of days and be done with it, I would have done it by now. It is actually a hard problem.

It's not clear to me that solving it is worth the engineering effort, though I certainly see that some feel very strongly that it is.

@larsbs
Copy link

larsbs commented Jun 12, 2023

@munificent I definitely don't have the answer to all those questions since I'm just a mere observer, but I have a couple questions more:

  • Haven't most of those questions been successfully answered by the prettier team (judging by the amount of developers that are pretty happy with it)?
  • Do you think they're facing different problems (and hence, need different solutions) than the ones we're facing in Dart?

@munificent
Copy link
Member

  • Haven't most of those questions been successfully answered by the prettier team (judging by the amount of developers that are pretty happy with it)?

Probably yes. Like I said, these are solvable problems, but engineering time isn't free.

  • Do you think they're facing different problems (and hence, need different solutions) than the ones we're facing in Dart?

They have followed a different historical path, which makes a difference. Any IDE or editor integrating Prettier has (I think) always been able to presume that it will need to be able to read some config file, and the integration was always designed around that.

For dart_style's entire existence, every editor, IDE, code generator, and other tool using it has been able to assume that the formatter does not need access to the file system or any other configuration data. Any implementation choices those tools made based on that assumption may no longer be valid.

Breaking that assumption without breaking those tools is potentially a significant amount of work.

@TimWhiting
Copy link

TimWhiting commented Jun 12, 2023

This isn't a single-PR-scale change. Implementing this only in dart_style would make the command line dart format behave differently from the IDEs almost everyone is using. Most IDE integrations format files that are in memory, not on disk. They use the formatter's library API to do that, and in that mode, the formatter should probably not be wandering around the user's filesystem trying to find a config file. The config itself may have an unsaved change to the line length which should be honored when formatting happens in an IDE.

Acknowledged, it would be best if these changes all happened at the same time, so that inconsistencies don't happen. I see a minimum requirement of dart_style, flutter / dart clis, vscode, IntelliJ, package:build, and package:source_gen. I acknowledge that all of these teams might not have the bandwidth, but I'm sure the community that cares about it has the motivation and time.

In fact, the dart_style library can't look at the file system without it being a major breaking API change. Currently, the dart_style library doesn't depend on dart:io, which allows it to be used on the web, Flutter, or other platforms or contexts where dart:io isn't available.

This points to a design requirement that it be implemented in the cli tools, not in the format code itself. There is already some io specific tools in the dart_style package: here. Seems like a good place to put the logic, and since it would be in the package, that logic would be reusable by dart cli, flutter cli, package:build, and package:source_gen.

To add support for this, we would have to coordinate the design with at least the IntelliJ and VSCode plug-in folks. They would need to add support for finding this config file through their own file system abstraction layer and sending the correct line length for each file when using the formatter's library API. I don't know if they have the bandwidth for that. Other editors with less active support may end up broken or inconsistent until/if they are updated too.

Agreed @DanTup, looping you in here. Would you have the bandwidth to review a PR if someone were to implement this for Dart-Code?
As far as other editors go, if we can keep the logic small i.e. one config file, simple one line configuration which can be imported from dart_style/io.dart (this would need exporting a small interface) I feel that the community could work on support for editors that don't have official flutter support.

There are design questions around the configuration itself:

  • Does the configuration go in the pubspec? build.yaml? analysis_options.yaml? Somewhere else? What is its schema? How are configuration errors handled?

Using the pubspec.yaml is in the title of this issue. I know pubspec has grown beyond it's original design, but limiting it would break many plugins that I know of, so although it seems odd, I see no technical reason for not putting it there, though it would not be my first choice. build.yaml seems odd because it is typically only used when using code generators, and even then rarely, and with macros, more people might not end up using generators anymore. Generator specific line lengths could still be supported, in which case I would put those generator specific overrides in build.yaml, and the support in each of the generators themselves and only if they wanted. analysis_options is definitely related due to the linter warning about line length, it makes sense to have the configuration option next to the configuration enabling such a lint. As such I would lean towards an analysis_option.yaml configuration. As far as the format for the option I see no reason not to just keep it simple:

formatter:
  line_length: number
  • What happens if there are configs at multiple levels of the directory hierarchy? Does the one nearest to each file being formatted win? Or does the config in the same directory where the formatter is invoked win?

With the reasoning above about it being related to the linter configuration in analysis_options.yaml I would use the same analysis_option resolution.

  • How do code generators that use the formatter to format their results know what line length configuration to use? Who will own updating those generators to determine and pass in the right line length?

Ideally generators do not need to worry about it unless they provide their own custom builder, or an override for the default line length. I would personally add the logic to just package:build, and package:source_gen as a default for any generated file ending in .dart. (Maybe add an option to opt out).
For example source_gen already supports a formatOutput option for builders here. This defaults to using dart style here. If dart style provides an io specific export that allows you to resolve formatter options for particular file paths, this could be updated to apply those options.

  • What happens when file IO or permission errors occur when looking up the directory hierarchy to find the config file? (These do indeed happen.) What if those errors are intermittent?
    (Note that it's not enough for the implementation to handle this. The PR would have to include tests of it. Writing file IO tests is time-consuming, annoying, and really hard to get working across all platforms. But if we don't have those tests, then it's too easy to regress that behavior.)

Agreed. File tests are buggy sometimes, but definitely necessary in this case. As for how to gracefully handle an error I would say emit a warning, and do not format. At worst, some updated code is not formatted (sometimes happens with VSCode anyways if the analysis server times out on format requests). However if it were to just use a default that could end up changing a lot of code that shouldn't be changed.

  • When looking for a config file, does it stop walking the directory hierarchy at some point and give up? (For example, Black stops when it sees a .git or .hg directory.)

Again, use analysis_option resolution. I think it is reasonable to stop early at a .git directory.

  • What happens if the user passes a --line-length argument and there's a config file? Does that command line option specify a default or an override?

Emit a warning, user should update the file so that the rest of their team can format the same as them. If they only specify a single file and not a directory, I would say treat as an override, but still emit a warning saying that the next time the file is formatted it will be changed unless you update the format config file, or manually override again.

  • What happens when users want to configure the line length differently for different parts of the same package? Is there a mechanism for this? Do we end up also needing per-file configuration?

The analysis_option.yaml file allows for ignoring / excluding files, and can be nested under directories. Use analysis_option scoping mechanism. Maybe formatting should have a separate exclude / include section - or most likely just an include option that overrides any excludes. Typically users exclude generated files, but I can see them wanting generated files to be formatted for readability of those files.

  • If we find a config file that doesn't configure the line length, should we keep walking up the directory hierarchy looking for a surrounding one that does? Is the absence of a line length configuration treated as "configure this directory's files to the default" or "use any surrounding configuration"?

Yes, up to the package root. I'm assuming this is how analysis_option.yaml file works.

  • Assuming this goes in the pubspec, is there a way to configure the line length for a directory without creating a pubspec? Should there be?

I'd prefer analysis_option.yaml due to how it answers all of the previous questions well, and addresses the scoping / including file mechanism.

I'm sure all of these are tractable, but are decisions that would have to be made. I'm sure there are other design questions I'm not thinking of.

Happy to try answering any new questions

As far as where to put the bulk of the resolution logic:

Given my leanings towards analysis_options.yaml, it would make sense for the analysis_server to expose a way to query options for line-lengths for specific files, which would simplify the editor plugin logic for those using the analysis_server, especially if it uses analysis_option.yaml based scoping. And dart_style already has a dependency on analyzer, which seems to fit perfectly. So the ultimate logic for resolution would be added to analyzer, with a small interface reexported from dart_style potentially (though package:build already depends on analyzer if I'm not mistaken), so that might not be strictly necessary.

Also, we could lean on the file based tests for analysis_options.yaml, with some additional tests for formatting specific configuration.

If this was something I could just hack together in a couple of days and be done with it, I would have done it by now. It is actually a hard problem.
It's not clear to me that solving it is worth the engineering effort, though I certainly see that some feel very strongly that it is.

Thanks for recognizing that some of us feel strongly that it is worth the effort. Let's say a group from the community had open PRs for

  • dart-lang/dart_style
  • dart-lang/build
  • dart-lang/source_gen
  • dart-lang/sdk
  • flutter/flutter
  • dart-code/dart-code (vscode)
  • IntelliJ -- wherever the plugin is

would that be acceptable? (obviously pending buy-in and approval from the vscode / intellij plugin authors as well).

I recognize that the proposal of putting it in package:analyzer, means that ultimately it becomes a dart team maintenance burden. I hate to do that to the dart team especially when they have so many other priorities such as working on new language features. My hope is that formatting specific issues and maintenance could be annotated with a 'community:help' label, so that we know what issues the community can help with to give the dart team more time to work on nice language features and improvements to the compiler.

@DanTup
Copy link
Contributor

DanTup commented Jun 13, 2023

Agreed @DanTup, looping you in here. Would you have the bandwidth to review a PR if someone were to implement this for Dart-Code?

The formatting in Dart-Code is handled by the analysis server, so any work to support this would need to be done there (https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/lsp/handlers/handler_formatting.dart). The server already supports setting the line-length, but it's currently LSP-specific way so running "dart format" won't read it.

Btw, there's a somewhat-related issue at Dart-Code/Dart-Code#4482 about a "do not format" setting that Dart-Code used to have but doesn't work with LSP. The request is to be able to exclude some set of files from formatting and I suggested a formatter section in analysis_options.yaml (ofc Bob's caveats/concerns about reading files above apply here too) - if these things were to be implemented, it would make sense to try and align their config.

@TimWhiting
Copy link

@DanTup
So a formatter section of the analysis_options.yaml, with a filtering excluding / include list, would work for that issue as well.

formatter:
  line-length: 120
  excludes: **/*.g.dart # Or whatever

As far as I know the analysis_server doesn't have qualms about reading files right (from the resource provider of course)?

but it's currently LSP-specific way so running "dart format" won't read it.

As far as I understand it, this is the main issue. We need a package that both dart format / analysis_server can depend on for interpreting the options of an analysis_options.yaml file. They both depend on analyzer. I think this is an ideal location to put it.

@DanTup
Copy link
Contributor

DanTup commented Jun 13, 2023

As far as I know the analysis_server doesn't have qualms about reading files right

Correct, the server knows where pubspec.yaml/analysis_options.yaml are and can easily read them.

I'm not familiar with how the "dart format" command calls the dart_style API, but I wonder if the "dart format" command (which presumably lives in dartdev) would be a reasonable place to handle any config on disk, then provide arguments to the dart_style API?

Edit: Actually, dartdev doesn't have the implementation of the "format" command, it delegates directly to one in dart_style:

@lukepighetti
Copy link
Author

lukepighetti commented Jun 13, 2023

I fear that this high barrier to entry will lead to ecosystem fracturing. Dart Code Metrics now has project level configurable line length for Dart code formatting, and they have very quickly implemented custom formatting options based on developer feedback. I think both of those things are hugely beneficial, but they are now in an external formatting tool. I would rather see these features inside dart-fmt but developers will choose the path of least resistance to getting the tools they desire for their projects. It would be easy to say "go use DCM if you want these features" but I believe they exist in response to a real need that is not being met, and should be met, by core tooling.

Dart linters are a great example of where I believe the Dart team exceeds expectations when it comes to community contributions. It's very easy to submit a new lint rule and it's well understood that they aren't internal recommendations. Just tools that can be opted into. I wish dart-fmt had a similar ethos.

I do believe that a slight change in dart-fmt ethos would also open the door to flutter/flutter adding automatic formatting to their repo which would greatly benefit people trying to contribute. I think this is a major pain point that is worth resolving.

I'm also optimistic that the proposal above can be achieved with minimal impact to downstream consumers.

@Nikzed
Copy link

Nikzed commented Sep 22, 2023

I feel like it should be implemented as project property for sure, kinda hard to explain every new person to set line length to 120 each time, while doing code review

@Topbrains
Copy link

We need to be able to choose our style and select whatever width we want. For corporate development there are plenty of tools that will enforce length and formatting standards.

How come a group that does not dare being opinionated about state management, is so opinionated about formatting?

@allanlaal
Copy link

dartfmt should respect .editorconfig (and that includes indent_style)

we no longer have to suffer, just because M$ Visual Studio chose 80chars and spaces and it trained a generation of devs to not know these can be changed

#customization is king and also offers #accessibility

@exaby73
Copy link

exaby73 commented Mar 29, 2024

4 years later... No labels, no assignees... Looks like this is a dead issue and we'll never get this. At least make the hard limit to something reasonable like 100 chars instead of 80. We aren't in the age of square CRT monitors where the 80 char limit originated from. 100 chars serves well for 16:9 displays, with 120 to 150 for ulta-wide ones

@hbock-42
Copy link

I am afraid of the moment I will not be solo on the mobile (flutter) part at my job. How will I enforce the 100 line-length I am using?
I like your work @munificent , even got your book about programming pattern applied to game dev from before flutter existed, but on this one I have not seen any good point about preventing dart users to set line-length on a per project basis.

To sum up:

  • Keep 80 the default
  • Make it possible to set line-length per project (analysis_options, pubspec, who cares)
  • Maybe don't advocate / advertise this feature so only people that really wants / needs it use it.
  • Lower the score of package not following the 80 line-length rule on pubdev.

In the meantime, do you guys think it would be possible to make a package that once installed, when build_runner is run, modify the vscode and intellij config to change line length?

@natebosch
Copy link
Member

In the meantime, do you guys think it would be possible to make a package that once installed, when build_runner is run, modify the vscode and intellij config to change line length?

This isn't something I would recommend. build_runner can output any file into your package directory - so it could output IDE config as long as the IDE allows reading the config from the project directory. It's unlikely that the IDE config would change in reaction to a change in some other file though. If the IDE can be configured by a file in the repo, consider writing that file once and checking it in to the repo.

@exaby73
Copy link

exaby73 commented Apr 4, 2024

@hbock-42 You can still sort of enforce it. You can make VsCode and IntelliJ project configs available in the repo so that which ever IDE your team uses, they will have both configs with the line length set to 100. On top of that, you can have a CI step that checks this using the following:

dart format --line-length 100 --set-exit-if-changed lib

Hope this helps you :)

@hbock-42
Copy link

hbock-42 commented Apr 6, 2024

 You can make VsCode and IntelliJ project configs available in the repo so that which ever IDE your team uses, they will have both configs with the line length set to 100.

For intellij I think I found a file that might be the right candidate: Project.xml.
Inside this file there is a section that seems to be related to dart line length (need to make some test to validate this is not a random 120 that has nothing to do with the 120 line length I set in the IDE).
The file codeStyleConfig.xml should probably also be tracked by git because of the PREFERRED_PROJECT_CODE_STYLE that can be set to Default (which is the user IDE default code style). We want it to be Project (again need to check if this is really the setting I think it is referring to).

So for intellij users, this would only change IDE settings for the project, and only enforce line length (which is what we want - we don't want to enforce other rules).

Regarding vscode, I will have to make some research (I personally don't use it when doing flutter stuffs), I hope there is a file that contains only difference from the default user config we want to enforce, and not a file containing all vscode settings.

@crimsonvspurple
Copy link

It has been discussed already. Just commit the Project.xml in your git repo for IntelliJ (Dart right margin) and settings.json for VSCode (dart line length).

@benthillerkus
Copy link

Just saw this tracking issue for the upcoming tall style #1402 (comment)

And some of the remaining tasks

The FormatCommand class which drives the dart format command-line tool and accesses the file system directly should determine the language version of each file it processes. This means looking for either a pubspec or package configuration file in surrounding directories to determine the default language version.

and

Talk to the analyzer and other IDE integration folks about updating their integration to use that new API and pass in language versions when requesting formatting.

sound a lot like the concerns raised against this configuration option.

It seems specifying formatting options in the pubspec is now viable?

@natebosch
Copy link
Member

It seems specifying formatting options in the pubspec is now viable?

It's a smaller lift than before, but it's not trivial. Users of this library outside of dart format use other mechanisms than pubspec.yaml to find the language version. Consistent output is a requirement, and making the language version mandatory is a risk, but a smaller one than requiring formatting specific configuration information.

@munificent
Copy link
Member

Yes, it's definitely more viable and something I hope to do (which is why this issue is still open). However, we won't be able to reuse much if any of the code used to find the language version. We pull the language version from the .dart_tool/package_config.yaml file, and there is an existing package which does the work to walk the parent directories looking for that file.

For configuring the line length, I think the right place to put that is in the analysis_options.yaml file (or maybe the pubspec), so we'll need some different code to find that. Also, we'll have to make sure that all of the widely used tools that use dart_style as a library also correctly read that configuration and pass the proper line length in. Otherwise, you'll get different formatting results depending on whether you format within your IDE or from running dart format ....

@IamRezaMousavi

This comment was marked as duplicate.

@munificent
Copy link
Member

munificent commented Oct 4, 2024

I'm going to close this as a duplicate of #833 which is older and has more reactions on it.

You may want to note this comment: #833 (comment)

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

No branches or pull requests