Tall style: It shouldn't be the responsibility of Dartfmt to automate commas. Consider using lint rules instead #1539
Comments
|
I have tried the new formatter on a large code base, and I love the automatic addition and removal of comma's. I only found one case where I didn't agree with the tall style decisions (#1540). In all other cases I am happy with the changes. So FWIW: I am strongly in favor of automating the addition and removal of comma's. (Side note: Maybe if we had lints with auto-fixes, we could do the automatic addition and removal of comma's there. But you'd probably need the whole implementation of the formatter in such lint to decide if something fits on one line or not.) |
|
As I said, I'm also for automating it. That's not my point. |
|
having dartfmt automatically handle trailing comma always seemed like one of those ideas that sounds better than it is. a lot of decisions about formatting are controlled by these trailing commas and im not convinced any one algorithm will produce good results in all cases. meanwhile as you say, a configurable linter does a great job of managing this |
|
It also doesn't solve the problem of "no more style issues during reviews", and can also make refactoring worse. For example, the following: final content = json.decode(
await rootBundle.loadString('assets/configurations.json'),
) as Map<String, Object?>;Now formats as: final content =
json.decode(await rootBundle.loadString('assets/configurations.json'))
as Map<String, Object?>;I don't think that's acceptable to me. So I'd end-up refactoring my code to: final content = json.decode(...);
content as Map<...>;
// TO-DO use contentEnd result?
|
|
I really don't know what to say here. Literally the first sentence in the proposal for the new style (#1253) is:
Yes, but that's true of every other whitespace change the formatter applies. Given an expression where there are multiple places it could split, the rules behind choosing which places to split are also very complex. In fact, they are the same rules. The formatter already has to decide whether an argument list or collection literal should split or not. The only real differences compared to the old formatter with respect to commas are:
Again, this is exactly true of all whitespace changes as well. The goal of the formatter is not to produce code that is more beautiful and readable than a diligent human spending unbounded time and using the full context of the surrounding code could hand format. The goal is to get close enough to that high bar that the remaining difference in readability isn't worth the large human cost to get it. It's like driving a manual transmission. A skilled driver might drive better driving a stick. But is that the most valuable thing they can be doing with their time?
I am well aware! 🫠 That's why I have gone through an absolutely laborious process over the past year to determine if moving to this style is a net win for the entire Dart community. We have pretty clear feedback that it is.
Yes. Again, this is equally true of whitespace. But I have found in practice that context rarely makes that big of a difference in how someone would format a piece of code outside of a few edge cases like tabular data in collection literals. I really don't know what to say here. Trailing commas are effectively whitespace. They have absolutely no semantics in the language. Your argument against automating them is, as far as I can tell, an argument against automated formatting in general. Which, sure, if you don't like automated formatting, that's fine. But if you do want to use an opinionated automated formatter, you may as well let it handle your trailing commas too.
One of the main reasons they are not is because of reversibility. For things like braces on control flow constructs and Basically, there is a line where we say everything below this is not semantically interesting enough to be worth requiring a human to hand maintain it. Everything above that line the formatter doesn't touch. Everything below that line, the formatter does. What I think leads to a particularly bad user experience is when something crosses that line. If humans are able to author some formatting choices, but the formatter can output those same choices too, then you end up in a situation where the formatter can't tell "Did a human put this here, or did I put it here the last time I was run?" It's not a good tool if the formatter can create a mess that a human doesn't want, and then not even be able to clean up after itself. The new style is explicitly designed to avoid that. We know from asking users and looking at how they format their code that most prefer multi-line argument lists to have trailing commas. That implies that when the formatter splits an argument list, it should add a trailing comma. Otherwise, if the formatter splits the argument list and doesn't add the trailing comma, it's just leaving a pointless task for a human to perform. But if the formatter adds trailing commas but doesn't remove them, then, again, it ends up leaving work behind for a human. The high level question I want to ask is: Today, when you decide to insert a trailing comma to manually force an argument list to split that wouldn't split otherwise, what process is going through your head? What is the criteria you are using to decide that, "Yes, it would fit, but this one particular argument list looks better if it's split anyway."? I'm not asking rhetorically. Literally, what are the heuristics you're applying? And then, once you've answered that... can the formatter just implement them? If we had a version of the formatter that could literally read your mind and would split every argument list you wanted it to split, I presume you would be happy for it to add and remove the trailing commas. How close can we get the formatter to that? Are the rules so complex and subtle (and the effect on readability so massive) that it's simply impossible to automate them? Or can we get close enough and then you can focus on the actual semantics of your code instead of worrying about commas and newlines? |
|
There is a way to force a trailing comma: add an empty comment to the construction: void main() async {
fn(
//
param1: 1,
param2: 2,
);
final map = {
//
'a': 1,
'b': 2,
};
}This has long been the way to preserve line breaks in list and map literals. Since most regrets come from the formatter removing the trailing comma rather than adding it, this trick should cover a lot of regressions and maybe it should be promoted. |
|
@munificent I do want automated commas. I'm saying to move your heuristic to the analyzer_server instead of inside dart_style. We should be discussing lint vs dartfmt instead. Not "should we automate commas or not".
People couldn't play with it until now either :).
I'm not sure. I think some people would argue differently. For example, @nex3 argued for automating function bodies: #1532 (comment) Similarly the very reason why this issue exists is because I am unhappy that the formatter removed a comma. To me, dartfmt is inconsistent. We want to automate commas, regardless of if removing them makes some users unhappy.
I'd be happy if Dartfmt matched exactly what I want. Although I'm more than happy to share my heuristics. I already did so on multiple occasions, in other issues. For example, "add a trailing comma if a multiline closure/list literal isn't the last argument". #1528 (comment) Anyway as I said, IMO we should be discussing lints vs dart_style as a way to automate commas instead. |
I think this is deeply inaccurate. Just take a look at the FAQ: the three highest-priority goals are consistency, ending debates, and freeing users from thinking about formatting: all things that are accomplished better by doing the same thing everywhere than they are by pleasing everyone. In case that's not explicit enough, the very next header is "I don't like the output!" which points out that that "may be a deliberate stylistic choice of the formatter that you disagree with". The header after that discusses why the formatter is not configurable, and that "the goal of dartfmt is not to automatically make your code look the way you like. It's to make everyone's Dart code look the same." The formatter never tried to please everyone. The only reason there was ever an exception for commas was because Flutter-style code was unreadable without something like tall style but there wasn't yet consensus on whether it was worth making everyone else's styles match. And this was a notable aberration: in essentially every other case, the formatter has always been strictly opinionated, prioritizing consistency over customizability, and over pleasing everyone. And there's substantial evidence beyond just the number of upvotes on #1253 that this is the correct approach, and one that's broadly popular with users over the long term. In the frontend world, by far the most popular formatter is Prettier which bills itself front and center as an "opinionated code formatter". Its option philosophy is clearly aligned with Similarly, Python's most popular formatter is Black, whose tagline is "the uncompromising code formatter". Its very name comes from the Henry Ford quote against customizability, "Any customer can have a car painted any color that he wants so long as it is black". Not only is pleasing everyone not a goal, trying to do it is actively harmful to the formatter's goals, and there's good reason to think that
This is true! In my ideal world, the only style choices I'd ever make would be which sub-expressions to pull out into variables (none of them 😈) and where to put empty lines. Even if all the style choices were the precise opposite of what I do by hand, the value of having them standardized and automated is so high that I'd still prefer it. But, critically, I also understand that different people have different thresholds for what they consider "semantically relevant" changes. I believe that reflecting the community consensus there—as with individual formatting decisions—is more important than meeting my personal preferences. I think 94.8% 👍s on a proposal that begins "you no longer need to manually add or remove trailing commas" is a very strong indication that most people don't consider trailing commas to be semantically relevant. About the specific question of whether this should be a lint instead: leaving aside the philosophical debate of whether trailing commas count as "formatting", the empirical investigation of whether the formatter can determine often enough what to do with them, and the pragmatic concerns about whether splitting this out would provide enough value to be worth throwing out and rewriting all the work that's already gone into handling commas in the Let's suppose for a moment your proposal is adopted. Consider the following code at width 20: // 20 char limit: |
void main() {
myFunction(
"foo"
);
}If you run // 20 char limit: |
void main() {
myFunction(
"foo",
);
}...which // 20 char limit: |
void main() {
myFunction("foo");
}Even if you're willing to bite the bullet and accept making ordering significant (and the corresponding failure of reversibility), consider this: // 20 char limit: |
void main() {
myFunction(
nested("foobar")
);
}According to // 20 char limit: |
void main() {
myFunction(
nested("foobar"),
);
}...but now you've gone over 20 characters! Okay, just run // 20 char limit: |
void main() {
myFunction(
nested(
"foobar"
),
);
}Oh no! We're in violation of the comma rules again! There's no way to fix this in general without running The core issue here is that trailing commas both affect and are affected by line length. Other fixable lints don't have this issue—even if they change the way |
Prettier is a lot closer to the current Dartfmt than the upcoming Tall style. For example: const x = { a: 42 };
const x = {
a: 42,
};Both of these are valid formats. Removing/adding a newline after the So Prettier is opinionated and offers controls over single vs multiline. What Prettier automates is ironically the opposite of what Dartfmt wants to automate. return <multiline>
<div>foo</div>
</multiline>;becomes: return (
<multiline>
<div>foo</div>
</multiline>
);So in a sense, Prettier and Tall Style are taking opposite decisions here. With regards to your examples about why the lint may produce incorrect outputs: As such, I disagree with the assumption that the lint would introduce a comma before the formatter can format the file. So given: // 20 char limit: |
void main() {
myFunction(
"foo"
);
}Then the linter shouldn't suggest anything. And formatting will remove the newline. And even if we don't do this change, this issue also assumes that we add a // 20 char limit: |
void main() {
myFunction(
nested("foobar"),
);
}The trailing comma would be flag as extra, and would be removed by Dart fix, wrapping whole function invocation in a single line. The key is that if someone manually adds a comma and doesn't enable |
|
Also quick note:
First, this isn't representative, as those 👍 are from before we could play around with Tall Style. I too upvoted the issue months ago. And if you look at all my comments in the issue starting from this one (#1253 (comment)), they all received a fairly decent amount of upvotes. In fact, one of the very last comment is about using |
|
I don't think you're actually addressing the points I'm making. The point isn't what Prettier's specific style looks like—different tools for different languages are always going to make somewhat different decisions. The point is that its philosophy is evidence of the value of opinionated formatters, and the high cost of pleasing everyone. (In point of fact, though, Prettier's documentation is clear that "The semi-manual formatting for object literals is in fact a workaround, not a feature. It was implemented only because at the time a good heuristic wasn’t found and an urgent fix was needed." This is not an example of desirable behavior worth emulating.) Similarly, it doesn't matter to what I'm saying whether or not the 👍s on the initial issue are specifically endorsing the current formatting (although I don't think 18 reacts are a reason to think many people are dissatisfied). The critical question is whether people think of trailing commas in principle as semantically meaningful, and the answer is clearly that they do not. Finally, if you need to have a linter's behavior know how |
I wasn't aware of the documentation saying that Prettier doesn't count this as a feature but a workaround. Fair enough. Although I'd be curious to see how the JS ecosystem would react if Prettier decided to remove that behaviour. I'm sure there would be a decent amount of pushback against it. But that's all theoretical.
It sounds like we have a significantly different interpretation of the events from the past week then. Although I base my opinion on a bit more than the issue. There have bit some discussions with various GDEs and co on private channels. My understanding is that:
Of course, all of that is subjective. But I don't think it's as clear-cut as you're making it out to be.
They share the heuristic. I don't think that's unreasonable. Viewing it as "a formatter option that's masquerading as a lint rule" is interesting. I agree that there's a decent amount of overlap. And to me that's consistent with other topics, such as other lint-rules or
Given: // 20 char limit: |
void main() {
myFunction(
nested("foobar"),
);
}Tall style would remove the comma and format it in a single line. So a The "dart fix + dart format" combo with lint rules is supposed to have the same output as built-in comma manipulation. If the outcome is different, the lint rule was likely incorrect. |
|
I think we have wandered off the path into philosophy. Earlier, I asked:
I still don't have an answer to that. @rrousselGit, if you want explicit control over splitting argument lists... why? Can you show me examples where you think the current tall style formatter is doing a bad job? For those examples, can you explain why it would be impossible to tweak the formatter's splitting heuristics to do a better job? |
|
@munificent I already answered. See #1539 (comment) (the last paragraph). Dartfmt has always been opinionated, but it's not a dictator either. It has some jiggle room. Even if I raise an issue, there's no guarantee that Dartfmt will fix it. Let's not forget that Dartfmt has a monopoly over formatting. Folks upset by Dartfmt changes don't have many alternatives. That jiggle room is currently the only way to move forward for folks who don't like a specific formatting. And as I said multiple times: I dislike how inconsistent about customisation Dartfmt is. For example, whenever a IMO it should be all or nothing. As it is, I don't really feel the benefit of automating commas. Because I still have to make the mental effort of going through all changes to validate that I don't need any further change. |
That's not accurate. Formatting this on a single line would produce: // 20 char limit: |
void main() {
myFunction(nested("foobar"));
}which is well past the line length limit. The only correct tall-style formatting, and what the prototype actually produces with // 20 char limit: |
void main() {
myFunction(
nested(
"foobar",
),
);
}There's no way for your proposed separation of concerns to produce this output without either making the formatter comma-aware or making the linter formatting-aware. Similarly, you can construct cases where there's a comma that the current tall style would remove, but that the linter can't possibly know should be removed without running a full formatting pass. (In case you're willing to bite the bullet on that as well: note that it takes |
OK, I'm working on #1528 and related issues now. If those are fixed, does that change how you feel about automating removing commas? |
|
It would be more bearable. But I still think that makes Dartfmt a bit of dictator, and still believe that Dartfmt is inconsistent in that regard. I'm not fan of how it was argued not to automate |
|
I strongly oppose automatic trailing commas. When writing complex programs in flutter, it is very easy to form deep nesting. At this time, reasonable indentation is the most important. I don't want my code to be occupied by a lot of meaningless brackets. |
|
I'm going to go ahead and close this because I don't see anything concrete here that I can take action on. The overall proposal to automate adding and remove trailing commas has already gone through a thorough change process and we have clear feedback that the community overall approves of it. I am of course 100% interested in other issues being filed for how we can approve the style that the formatter applies, but I believe the overall question of whether it should remove commas is settled. |
Hello!
In my opinion, the Tall style changes tries to do two completely distinct things.
Both goals are valuable. But in my opinion, it isn't the responsibility of Dartfmt to add/remove commas. That's up to the linter. For example, by enabling require_trailing_commas.
I propose the following plan:
unnecessary_trailing_commalint rule, which would be comparable torequire_trailing_commas. This enables the so-called "reversability" of the styleguide discussed in other issues.require_trailing_commas/unnecessary_trailing_commaaccordingly to match the new style.unnecessary_trailing_commaandrequire_trailing_commasto core lint rules, such that they are enabled by default on all Flutter projects.dart fixon save/format)() =>vs() {};forvswhile; earlyreturnvsif/else;switchstatement vs expression ; ...This would achieve the same goal as automating commas through DartFmt.
But lint rules can be toggled. New rules can be added at any time. And rules can do much more than adding or removing a comma (for example sorting named parameters, refactoring to early returns instead of if/else, refactoring between for/while, ...)
Why I think it is a bad idea to have the formatter deal with commas
IMO part of the issue of dealing with commas in the formatter is:
The rules behind when a comma should/shouldn't be added can be very complex. There will be the equivalent of "false positives" – where the formatter decided to add/remove a comma in a place where there is a more appropriate solution.
This strictness is useful for raising floor of how readable Dart code can be. Unreadable Dart code will likely become more readable with the Tall Style.
But due to false positives, this strictness also lowers the ceiling of how readable Dart code can be. Nicely formatted code today can become worse.
At the same time, fixing the formatter when there is an issue can be difficult. Millions of lines can be impacted.
Also, style guides can also be context dependent. A Flutter Widget may want to use different style rules than pure Dart code.
And to a lesser extent, packages may also come with their own styleguides within the confines of their public API. Yet the formatter cannot realistically decide to format things differently based on that context.
Lint rules do not suffer from those issues:
Similarly, lint rules are already commonly used for style guide purposes. While some diagnostics can be categorised as "here's a possible bug", many diagnostics are purely stylistic.
For example, we have diagnostics to:
import relative.dartvsimport 'package:my_package/absolute.dart'vs"vsr'vsr"based on the content of a Stringiterable.forEachvsfor (final a in iterable)...
And many of those capability have are more or less related to formatting.
Here are some examples:
All of those rules can be considered fairly similar to trailing commas.
In fact, I could see a world where someone would argue that those lint rules should be built directly into Tall Style.
The point being:
Lint rules offer a much higher degree of flexibility for enforcing styleguides while offering a similar developer experience.
If we were to enable
require_trailing_commasand the non-existingunnecessary_trailing_commaby default in the Dart/Flutter SDK, we would still solve the "cons" listed in the Tall Style rfc.But we wouldn't lose out on the "pros". And we would be much more reactive to whatever issue that arises.
The text was updated successfully, but these errors were encountered: