dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
647 stars 120 forks source link

allow configuration of line length in pubspec.yaml #918

Closed lukepighetti closed 1 week ago

lukepighetti commented 4 years ago

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
chenasraf commented 1 year ago

If anyone is interested in a workaround, you can use a package like script_runner (disclaimer: this is my package).

  1. Install globally:

    dart pub global activate script_runner
  2. Add a script runner with the CLI args you want:

    # pubspec.yaml
    # ...
    script_runner:
      scripts:
        - format: dart format --line-length 100
  3. And run it using scr instead of dart:

    $ scr format .
dancamdev commented 1 year ago

All I see here is needless resistance to a feature that would benefit many and detrimental to none.

You can still keep the default 80 line length limit, while allowing for others to change it as they please.

This is objectively an issue for many, project wide line length setting is IMO a basic feature that other ecosystems are solving efficiently - such as prettier using a prettierconfig.json.

Having something such as a 'dartfmt.yaml' would solve the issue and allow for configurations which I feel should be basic behavior for a formatter in 2023

flukejones commented 1 year ago

Even the Linux kernel has deprecated the 80 char limit https://www.phoronix.com/news/Linux-Kernel-Deprecates-80-Col

THREE YEARS AGO!

caseycrogers commented 1 year ago

I don't know if I have much to add other than +1 to allowing us to customize line length. For diffs, I already find split diff mode unreadable and never use it over unified. For writing code, I find 80 painfully short and really annoying to deal with, especially for Flutter code.

I get and respect the goals of consistency and short circuiting style debates on individual teams, but that's a tradeoff and I think line length is one of the places where the loss to consistency is justified by the gains to usability. All that said, I'd also be happy with deprecating the 80 char limit default and moving to a fixed 100 or 120 char limit and still not allowing customizability-it's more the 80 that I have a bone to pick with than the inflexibility.

One other thing I will say (which I saw elsewhere-I think from another Github issue) is that newspaper readability studies fundamentally do not translate to code readability. Namely, in a newspaper, the reader has to scan the entire line width to read the line. In programming (ESPECIALLY in heavily nested Flutter), the start of the line is often a ton of whitespace for indentation that the reader need not scan to parse.

gslender commented 1 year ago

Yep, as maintainers of Dart, it feels like they don't do any Flutter development and have no clue what the big deal is having to make sense of totally unreadable 80 line length code.

Where else is Dart so heavily used more than in Flutter? Who's the folks that are benefiting from limiting the line length to 80?

It's a shame Google doesn't take its projects seriously as they'd be a greater force in the market place if everything wasn't addressed in academic ways.

esDotDev commented 1 year ago

What I find most odd about this decision is that the rationale of saving developers time, is so clearly not being served.

In an attempt to avoid any discussion among developers at the beginning of a project, this instead:

modulovalue commented 1 year ago

I would highly recommend to anybody having issues with the 80 char limit to open a separate issue with example code and a link to this issue for reference. Remember that increasing the max line width might not be the best solution. The problem could be somewhere else. In other words, consider highlighting a problem, and not necessarily a solution.


it feels like they don't do any Flutter development and have no clue what the big deal is having to make sense of totally unreadable 80 line length code.

Are you using optional trailing commas? In my experience they solve most of the issues that people have with the 80 char limit. Of course, not all of them, but even if the limit was 120 or 1200, increasing the max line width would not end up solving all of them and introduce new ones.


Thank you Bob for not giving in. I agree with you, I think that changing the default or making it more easily configurable would, in general, not make dart developers more productive.

caseycrogers commented 1 year ago

I figured sharing hard examples would probably be helpful here. As an experiment, I jumped into whatever part of my project I was last editing, took a screenshot, and then evaluated what places I feel 120 would improve readability: Before (click to expand, I didn't want to pollute the issue thread here too much):

Screenshot 2023-06-08 151018

After:

image

The three problems in this random snippet fit into two categories:

  1. A function signature that is just a bit too long, so I have to add an optional trailing comma and split each argument onto it's own line. If the function had more than just two arguments, I'd find the multiline layout more readable, but not here. This one I must say is pretty 50/50. The line does get a bit overwhelming in the after picture.
  2. Variable assignment that gets bumped onto the next line. I think this really hurts readability because the connection between the expression on the right and the variable on the left is a lot less visually apparent while skimming.

In general, each line split up by the char limit adds vertical space to my file which makes it a lot harder to navigate. I have empty space to the right so I'd love to use that empty space to reduce the amount of scrolling required and to get more code visible in the viewport at once. And, this screenshot was already taken with my IDE layout that has a lot of other UI elements competing for horizontal space. Listed from left to right:

  1. Project structure navigator
  2. One sub-panel with files and tabs for editing (this is the screenshot)
  3. A second sub-panel
  4. The emulator

So if I'm already liberally using my horizontal space (on a 15" laptop) for other things and I still have room to fit 120 chars, it really makes me wonder how common it is that someone can only fit 80 chars?

I encourage others to share their examples (either for or against a 120 char option) so that we can get concrete examples to work with here.

munificent commented 1 year ago

Yep, as maintainers of Dart, it feels like they don't do any Flutter development

I am a readability mentor for Dart in Google. I have reviewed hundreds of thousands of lines of code in thousands of changes written by hundreds of engineers across dozens of applications. An increasing fraction of that code is in Flutter applications.

I review Flutter code for its readability almost every work day. For several years, I was the most prolific readability reviewer, which means I probably looked at more Dart code than any other Google employee.

In addition, as part of my job on the language team, I cultivate a corpus of tens of millions of lines of open source Dart code that I constantly analyze and skim through as part of the process of evaluating potential language changes. I have looked at a lot of Dart code written by many people.

I would highly recommend to anybody having issues with the 80 char limit to open a separate issue with example code and a link to this issue for reference.

Yes, concrete examples of code that could clearly only be improved by increasing the line length would be helpful.

In almost all of the Dart code I've reviewed where lines are heavily wrapped, what I usually see is long redundant identifiers and overly complex expressions. One reason I prefer 80 columns as a default line length is that it incentivizes authors to think about making their code actually simpler and clearer.

caseycrogers commented 1 year ago

In this vein, looking at my above comment, the biggest culprit is definitely DribbleDatabase.instance.

This should be assigned to a local variable at the top which will make the following lines shorter and more readable and mitigate the line length problems I'm having.

Edit: also I've noticed some mistakes in this code now that I've publicly shared it XD For example, the streak argument is never actually used...idk why the linter didn't catch that. I've got some fixing to do.

esDotDev commented 1 year ago

One reason I prefer 80 columns as a default line length is that it incentivizes authors to think about making their code actually simpler and clearer.

120 forces this same thought process as well, so this is not really a compelling argument. So does 60, or 40 etc. The question is what is reasonable and what becomes onerous.

I still think this comes back to work environment, do you use multiple 19 or 21" monitors as most production developers do? Or are you judging what is reasonable on a small 15" laptop?

On large monitors, the 80 char limit comes off as extreme, on a small laptop it seems reasonable. In this case this just becomes an argument for supporting easy configuration across a project, that way a team can make a decision based on their own work environments, and we're not trying to find some perfect number for everyone (which doesn't exist imo).

Instead we can set a number that works for our team of several developers, or in the case of an open-source package, whatever works best for the maintainer of the project.

rydmike commented 1 year ago

The discussion of what is more or less readable when it comes to line length, while academically very interesting, is irrelevant to this feature request.

It does not matter what I, Bob, Modestas, Luke, Casey or Shawn argues is better, worse, more or less readable.

What matters is what a software team has agreed to use and enabling them to automate and enforce that agreed line length on a project level.

The issues caused by not being able to enforce agreed line length of dart format on a project level, has been been exhaustively presented already.

Given a situation where a team has chosen to use another line length setting for dart format than 80 chars, the current feature gap causes unnecessary complications and frustrations for such teams and projects. It seems to me that finding a way to accommodate their preferences, would be in general interest of improving Dart tooling, be in-line with being open and inclusive, plus it would not take away anything from anybody.

This feature request does not propose to change the dart format default line length from 80 chars, nor does it propose any changes to the way dart format actually formats Dart code. It only requests that teams that have agreed to use another line length, should be able to do so on a project level.


What is more and what is less readable is of course academically an interesting topic. We could for example examine and discuss the code style of the Flutter project.

It does not use dart format at all, because the project has decided to format and break lines differently than what dart format does.

Flutter repo also uses extremely long lines, as and when it deems it appropriate to do so. I think this example of a 263 chars long line, is probably one of the longer examples:

Screenshot 2023-05-13 at 20 45 31

We could argue if this is readable or not, but it is what the Flutter project has decided to use.

If you try to submit a PR that uses dart format to the Flutter repo, it will be rejected, and you have to redo your PR following the Flutter repo formatting rules. This of course raises the bar to contribute to the project, since formatting submitted code cannot be done automatically at all. Which of course speaks to why dart format is typically welcomed by most projects and seen as a great tool, as it avoids such complications.

Having the capability to set line length setting on project level, would only strengthen dart format. Who knows, maybe one day even Flutter could use it.

lukepighetti commented 1 year ago

Just wanted to say that I have no interest in forcing other people out of 80 char. To be perfectly honest I would probably keep using 80 char. That said, there are projects using 120 char and even 140 char.

At work, we would use it to coordinate line length across Kotlin, Swift, Javascript, Go, Python which are all set to 120 characters. Dart is the only one we cannot configure at the project level. We can only configure it at the IDE level, and we (as a rule) do not force developers to use specific IDE settings. We only enforce/expect project settings. And they are not available to us.

So this issue is NOT about if 80 characters is good or bad.

This issue is about making sure we can configure --line-length at the project level. Whether that's in pubspec.yaml, analysis-options.yaml, or some novel dartfmt.yaml, is not clear to me.

@munificent, it would be helpful if we could get some clarity on if you would accept a PR to "close the gap" on this feature. And if the answer is yes, if you have any opinions on where this setting could live? Here's my proposal

Proposal

  1. enable the following (optional, default 80) entry in pubspec.yaml
dartfmt:
  line_length: 120
  1. expose the --line-length argument again within dart format --help with guidance pointing to this new entry

  2. (loosely) when dart format is ran, it looks down the file tree for pubspec.yaml to build context for sub-folders by checking for dartfmt.line_length. It also looks up the file tree for a parent. Context is applied/scoped in the expected way

I think that pretty much covers it?

gslender commented 1 year ago

So this issue is NOT about if 80 characters is good or bad. This issue is about making sure we can configure --line-length at the project level. Whether that's in pubspec.yaml, analysis-options.yaml, or some novel dartfmt.yaml, is not clear to me..

This is the point.

Hopefully common sense prevails and a PR is seriously considered along with the academic argument over whether 80 char line length could or should be used is left in the past where it belongs.

esDotDev commented 1 year ago

It's interesting to look at other languages to show how arbitrary this limit is.

Default line lengths: SwiftLint - 120 C# w/ Resharper - 120 Kotlin - 100 Go - 100 Python w/ BlackFmt - 88 Dart - 80

Of these, Dart is the shortest, and one of the only ones that does not allow you to set it on a project level. One thing is clear from these numbers: there is no perfect value.

Allowing you to change it, but then not define it at the project level makes a poor middle ground. It opens the door to custom line lengths, ensuring many(most?) teams will decide to change it, but then does not give us the tooling to properly do so, which forces a lot of wasted time.

If the goal is to not waste time, then going fully in one direction or the other would be more effective. Since the cat is already out of the bag with regards to custom line lengths, it seems the only logical option is to properly allow it to be set at the project level.

flukejones commented 1 year ago

Everybody commenting here wants the ability to configure the line lengths, regardless of their view on what is suitable.

But instead we are getting pigeon-holed in to an arbitrary line length that is essentially one persons opinion (and then they go on to argue by authority - we don't care, we just want the damned thing to be configurable by us).

modulovalue commented 1 year ago

Hi @caseyrogers,

Thank you for providing an example. I took a look and here are some thoughts on how I would deal with it.

I agree that it's not ideal that the formatter puts the await expression on the next line.

I would do the following to mitigate that:

That way, your code should start to fit into 80 chars.

For example:

final ... db;

...({
  required this.db,
});

...
  // 59 chars
  final cachedGuesses = await db.getStoredShotClockGuesses(
...

In short, I would suggest to avoid singletons and to consider making more use of type inference. Increasing the max line width limit would of course also help with your example, but I think that the alternative solution (i.e. to not use singletons) is much better.

gslender commented 1 year ago

This is getting to the point where it might be better for us to all escalate to someone senior in Google who cares more about the community - I can't believe line length is being argued as a mechanism to improve quality of the code - I must of missed that lecture at university!?

gslender commented 1 year ago

@kevmoo are able to look into this and provide some guidance around evolving Dart in a way such that the community appreciates the direction?

Daybayzayd commented 1 year ago

With line break :

00_80_line_length

Without line break :

01_80_line_length

It's more readable in the second screen, and the number of line is not increasing.

EDIT : Just like @modulovalue said, in my case, it was the "," that forced the indent... I didn't know this rule. But i have some other cases with method name and parameters than have more than 80 characters. :)

modulovalue commented 1 year ago

@Daybayzayd It looks like everything is working as intended with respect to the observations discussed in this issue. (Or maybe I'm not understanding what you were trying to bring across)

You can remove the trailing comma if you want to tell the formatter not to format Text widgets in a way that makes them span multiple lines.

//                                    ,- if you remove this, the formatter will not put your Text Widgets on multiple lines
//                                    v
Text("some text", style: _contentStyle,)
// Like this:
Text("some text", style: _contentStyle)
Daybayzayd commented 1 year ago

@Daybayzayd It looks like everything is working as intended with respect to the observations discussed in this issue. (Or maybe I'm not understanding what you were trying to bring across)

You can remove the trailing comma if you want to tell the formatter not to format Text widgets in a way that makes them span multiple lines.

//                                    ,- if you remove this, the formatter will not put your Text Widgets on multiple lines
//                                    v
Text("some text", style: _contentStyle,)
// Like this:
Text("some text", style: _contentStyle)

Thank you so much. I didn't know this rule. It works. I edited my post (and not deleted because i think if one like me is concerned by this problem, he can maybe see the answer :D )

jakemac53 commented 1 year ago

I think that pretty much covers it?

It is unfortunately somewhat more complicated because the formatter can also be used as a library, and is by code generators, and so we have to figure out how the configuration should be respected in those cases too. I think it's possible to resolve this but it is a complication (note that in this mode dart_style isn't even running on a file so it can't know what configuration to use, so code generators would have to be updated to pass in some configuration most likely).

Also in this case, code generators can't reach outside of package directories, so a global user config can't be respected (it would also violate the principle that code should be generated the same regardless of which machine its running on).

TimWhiting commented 1 year ago

I don't think it is a problem for the build.yaml file to also allow a global dart-line-length option. Of course the generators still need to respect this option, but I think the community would be willing to update the generators to use this option when formatting the generated files.

caseycrogers commented 1 year ago

Hi @CaseyRogers, ... In short, I would suggest to avoid singletons and to consider making more use of type inference. Increasing the max line width limit would of course also help with your example, but I think that the alternative solution (i.e. to not use singletons) is much better.

Thanks for taking the time to respond with your thoughts! I won't get too into it so as not to move too far away from the topic at hand, but I like singletons and have a code template that generates them with a bunch of helper methods. I also like being very explicit about types-I find the increase in verbosity is made up for by how much easier it is to keep track of what a variable is (there's even a linter rule for requiring explicit types). Especially the second point shows why configurable line length is desirable-linter rules manage trade-offs between terse code and explicit code, if a project's lints are set to value explicitness over brevity, the 80 char limit is going to be much more of a challenge than a project where the linters are set to value brevity more. If the ideal char limit is downstream of the lints, and the lints are configurable at the project level, then that seems like a compelling argument for allowing the char limit to be set at the project level.

WRT line lengths, I assigned my singleton to a local db variable and it solved one of the awkward line wraps-a definite improvement regardless of the max allowable line length!

Having the capability to set line length setting on project level, would only strengthen dart format. Who knows, maybe one day even Flutter could use it.

If we could get dartfmt to a place where the Flutter team would be willing to use it in flutter/flutter, this would be a MASSIVE win. The lack of auto-formatting is a huge barrier to contributing to Flutter. Maybe there's room for a cross Dart-Flutter team conversation about this with a brief public report written up about it? Namely, is there a set of changes made to dartfmt that would get the Flutter team to use it? If yes, are we willing to make those changes? With room for public input.

modulovalue commented 1 year ago

It looks to me like a big downside of Flutter using dart_style would be that every time Bob decided to approve a change, the world in Flutter-land would be forced to stand still for a moment as all contributors would have to update their PRs and learn about the new formatter changes. Whereas on pub.dev, there's at worst a pub-point penalty for not using dart_style, and everybody else is free to choose how much they want to depend on dart_style.

In other words, it seems to me that making the max line width configurable (via e.g. the pubspec file) would not help in convincing Flutter to adopt dart_style.


WRT line lengths, I assigned my singleton to a local db variable and it solved one of the awkward line wraps-a definite improvement regardless of the max allowable line length!

I'm glad that that suggestion was helpful. I agree that singletons can be useful and I agree that having explicit type annotations everywhere can be helpful (especially for understanding code on GitHub or in presentations where the dart analyzer is not available).


I also strongly agree with Jake. There are other code generators that are not build and like to use dart_style. Forcing them all to to have some custom configuration format for line widths and keeping them all in sync and teaching all users that they should keep them in sync sounds like a lot to ask for. And a new standard in the spirit of NO_COLOR (e.g. a DART_FORMAT=100 environment variable) could be problematic when working with other people (i.e. environment variables are generally not checked into source control systems so it would introduce sync issues between members of a team).

natebosch commented 1 year ago

Allowing you to change it, but then not define it at the project level makes a poor middle ground. It opens the door to custom line lengths, ensuring many(most?) teams will decide to change it, but then does not give us the tooling to properly do so, which forces a lot of wasted time.

Allowing a line length CLI flag does not "ensure" nor "force" it's use. If there are more ways that we can discourage use of this flag and save teams from wasting time we can certainly explore them. Removing the option entirely is not likely on the table because it's useful for tests, and it would be a significant amount of work for us to remove it and churn existing usage.

an arbitrary line length that is essentially one persons opinion (and then they go on to argue by authority

The line length reflects the opinion in practice of early Dart developers. It was not chosen by one person, and it is not solely by one person's authority that it isn't changing.

I think that pretty much covers it?

Other than hand-waving away the significant jump in complexity from not-configurable or aware of project layout, to configurable and knows how to navigate a project layout to find the config file, this is also missing consideration for all the ways that dart_style is used to format code outside of the CLI tool.

I don't think it is a problem for the build.yaml file to also allow a global dart-line-length option.

When this cascades out into requests across the ecosystem for any use of dart_style to support per-project configuration, it amplifies the cost.

Having something such as a 'dartfmt.yaml' would solve the issue and allow for configurations which I feel should be basic behavior for a formatter in 2023

The inevitable flood of requests we'd get for additional options in the file (which we'd have to turn down because we have no plans to increase complexity and write a configurable formatter) is another good reason not to introduce an entire configuration mechanism with a single configurable option.

esDotDev commented 1 year ago

Allowing a line length CLI flag does not "ensure" nor "force" it's use.

If you have a default that is smaller than all of your peers, and expose the setting, of course you are ensuring that it will be used by many developers. I don't see anything controversial in that statement.

Agree that removing it is not reasonably on the table, in which case it seems the only reasonable path forward is to finish what was started, and make the line-length properly configurable by project. This middle ground is the worst spot to be if you primary concern is saving developers time, which it seems is the core argument from Bob on this.

If we analyze this purely from a "dont waste developers time" perspective, the two best options are A) Don't expose the setting at all B) Expose the setting and allow it to be configured at the project level

Halfway between the two is the worst spot to be in terms of dev hours wasted. I would personally still prefer halfway over nothing at all, but I'm just pointing out the flaw in the analysis here. Once you have exposed it, then it only makes pragmatic sense that it should be configurable at the project level, the alternative is too messy.

TimWhiting commented 1 year ago

I don't think it is a problem for the build.yaml file to also allow a global dart-line-length option.

When this cascades out into requests across the ecosystem for any use of dart_style to support per-project configuration, it amplifies the cost.

The point is that it isn't a cost the dart team has to worry about. There are developers out there who care about this, and it actually isn't that hard to create a PR that passes the right options into dart_style.

Is the only option then for people who care about this to create PRs for all of the code generators out there (possibly having to fork them), and fork (or wrap) a version of the dart format / flutter format cli to take into account a configuration file, and add support in a separate vscode extension (or maybe even in dart-code) to use a different formatter for dart files? I hate to suggest it, because I feel like the community should feel like their opinions are heard and valid. But it does seem like a viable alternative if this issue continues the way it has been.

As long as there exists the option in dart_style to configure it, there is a way to make this nicer for developers. I just hope that a 1st party way to configure it seems more reasonable than having forks/wrappers of all these projects. I for one am tired of this issue just sitting here, and plan on contributing to the generators I use to allow this - side note, I've already done this for one code generator awhile ago (using an option in build.yaml), so I know the cost, and it is not much. The cost of having to turn on and off my user settings in vscode depending on what project I'm trying to contribute to (and revert 100s of changes that are just whitespace because I accidentally had a formatter turned on) is far more than updating a few code generators IMO.

I know this ignores the workarounds I mentioned for non-code generator tools. However, I have been playing around with LSPs and vscode editor apis, and I really don't think the wrappers around the other tools would be too hard to create either. In fact I'd probably just contribute the changes to fvm.

The inevitable flood of requests we'd get for additional options in the file (which we'd have to turn down because we have no plans to increase complexity and write a configurable formatter) is another good reason not to introduce an entire configuration mechanism with a single configurable option.

Yeah we get the message loud and clear. dart_style will never support more configurable options. If developers want more configurability they will need to create a fork of dart_style.

If it is really a problem of having a file that exists with only one option available, why not put it in the .dart_tool hidden folder and just provide a cli option dart format config --line-length=*, which edits that single option. This way it is obvious that there is no 'configuration file', there is just a command line configuration which just supports one option. The config file is completely hidden from the user. As a side note, in order to make sure users / contributors have the same configuration, we might have to add a negation !.dart_tool/format.yaml to the gitignore, so that that single file is committed to the project. I see this as one viable way forward.

lukepighetti commented 1 year ago

@munificent @natebosch @jakemac53 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. It would be a shame for me to invest into making this contribution and find out that it was known to be DOA by the team. Can someone give me clarification? cc @kevmoo

munificent commented 1 year ago

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:

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

@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:

munificent commented 1 year ago
  • 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 commented 1 year ago

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

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

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 https://github.com/Dart-Code/Dart-Code/issues/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 commented 1 year ago

@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 commented 1 year ago

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

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

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

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

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

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

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:

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

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

@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 commented 6 months ago
 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 commented 6 months ago

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

Just saw this tracking issue for the upcoming tall style https://github.com/dart-lang/dart_style/issues/1402#issue-2147897923

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

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

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 ....