-
Notifications
You must be signed in to change notification settings - Fork 170
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
[flutter] (prefer) trailing_commas #1338
Comments
FYI I have a request for |
I started a lint for this case and I think there should be at least 2 lints. One to lint missing coma in the following cases (flutter code that don't use void f(
int p1,
int p2
) {
....
} An other one (really harder to do) that takes some list of params/args/elements and advices to add a trailing coma to make the formatted code (with ``dartfmt) looks better. void f(int parameter1, int parameter2, int parameter3, int parameter4,
int parameter5, int parameter6, int parameter7, int parameter8) {
//
}
// after trailing coma
void f(
int parameter1,
int parameter2,
int parameter3,
int parameter4,
int parameter5,
int parameter6,
int parameter7,
int parameter8,
) {
//
} Another simpler lint could be to check trailing comas in widget trees. |
For the record, I'm not convinced that the value of a lint like this would justify the cost (cognitive load for users, maintenance cost for us). How many users would be helped by having this lint? (And for how long? Seems like after a couple of days most users would learn to add commas without needing a prompt.) That said, writing the lint should be fairly trivial. For every list you want to check, get the last element, get it's end token, and look to see whether the next token is a comma. If you only want to check lists whose elements are already on separate lines (or whose first and last elements are on different lines, or whatever) then use the tokens to get offsets and use the LineInfo to map those offsets to line numbers and compare those. |
That's what I did :) There are several problems. In the flutter codebase (that don't use In a project that uses |
Flutter would totally use this lint.
I've been writing code in this style for a couple of years and I still regularly omit the comma by mistake (then hate myself when I try to add another item to the list, and the compiler and analyzer lose their mind because they can't figure out what I did wrong). |
(Incidentally this is one case where using the formatter would make things ten times worse, because omitting the comma turns the entire list into a completely different style that is even less easy to add items to using copy and paste.) |
Good to know, thanks.
I think Flutter wants a rule that users should always include a comma at the end of parameter and argument lists. Is that true? But I don't understand the non-flutter / dartfmt case. What is the rule you're proposing for that case? |
(my feeling) 2 use-cases:
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
backgroundColor: Theme.of(context).canvasColor,
elevation: 0.0 // OOOOOOOOOOOOOOOOPS
),
body: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: <Widget>[
// Give the key-pad 3/5 of the vertical space and the display 2/5.
Expanded(
flex: 2,
child: CalcDisplay(content: _expression.toString()) // OOOOOOOOOOOOOOOOPS
),
const Divider(height: 1.0),
Expanded(
flex: 3,
child: KeyPad(calcState: this) // OOOOOOOOOOOOOOOOPS
),
] // OOOOOOOOOOOOOOOOPS
) // OOOOOOOOOOOOOOOOPS
);
} As you may have notice some trailing commas are missing. So when someone add an argument or a element in a list he could face errors described above by @Hixie. An other issue is that if someone copy/paste this code in a project that use Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
backgroundColor: Theme.of(context).canvasColor, elevation: 0.0),
body: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: <Widget>[
// Give the key-pad 3/5 of the vertical space and the display 2/5.
Expanded(
flex: 2, child: CalcDisplay(content: _expression.toString())),
const Divider(height: 1.0),
Expanded(flex: 3, child: KeyPad(calcState: this))
]));
} Here a good lint (for developers on flutter) would be to require a trailling comma if the closing parenthesis or bracket starts the line. (that's what I do in a lint I have locally - the 6224 fixes can be seen in a14n/flutter@08f7785)
|
Is this a fair summary?
It would be great to find a shared signal so that we don't need two rules, so let me ask a couple of questions. Why are some widgets (such as Does the style of formatting arguments on multiple lines apply to anything other than widgets? |
Here are some examples where you wouldn't want a trailing comma: void foo(String bar) {
}
var bar = [1, 2, 3];
quux(bar, baz); Here are some where you would: void foo(
String bar,
) {
}
var bar = [
1,
2,
3
];
quux(
bar,
baz
); |
My above sample (without commas for AppBar and Expanded) was code from a sample that should have used trailing commas. There are just typo that need to be fixed. So AppBar and Expanded are not exceptions. Regarding the samples @Hixie put in its last comment. It's what I described as the logic for the first rule and what I already have implemented locally. The second case is not only specific to Flutter. If you look at dart-lang/dart_style#753 it's related to angular. That's why I mentioned a hypothetical |
@a14n If you already have a something drawn up for use case (1), I'd really like to use it. Is it possible to factor the two lints you have in mind so they can be discussed/landed separately? |
@zanderso I just pushed a14n@39c59d6 available in https://github.com/a14n/linter/tree/prefer_trailing_commas branch I run the lint on flutter codebase (only on |
FWIW I think |
I think it could be very useful! Bad:void foo(
String bar,
) {
print(
bar,
);
make(
Foo(
a: 1,
b: 1,
c: 1,
d: 1,
e: 1,
g: 1,
),
);
}
void bar(int parameter1, int parameter2, int parameter3, int parameter4,
int parameter5, int parameter6, int parameter7, int parameter8) {
fn(parameter1, parameter2, parameter3, parameter4, parameter5, parameter6,
parameter7, parameter8);
} Good:void foo(String bar) {
print(bar);
make(Foo(
a: 1,
b: 1,
c: 1,
d: 1,
e: 1,
g: 1,
));
}
void bar(
int parameter1,
int parameter2,
int parameter3,
int parameter4,
int parameter5,
int parameter6,
int parameter7,
int parameter8,
) {
print([
parameter1,
parameter2,
parameter3,
parameter4,
parameter5,
parameter6,
parameter7,
parameter8,
]);
} |
Yep! 👍 |
It's September 18, 2020... almost a year since this commit opened, any update? |
|
If anyone is still interested in this rule we have implemented it in https://github.com/wrike/dart-code-metrics (along with other rules that can be useful). Any feedback is welcome. |
I've written a lint for this and have reproduced the test case below for discussion. Use trailing commas for all function calls and definitions. Exception: If the function call or definition, from the start of the function name up to the closing parenthesis, fits in a single line. Exception: If the final parameter is positional (vs named) and either a function literal implemented using curly braces, a literal map, a literal set or a literal array. This exception only applies if the final parameter does not fit entirely on one line.
|
How can I use this rule in my analysis_options.yaml file? It doesn't seem to be recognised as a rule as it's unreleased here. Is this rule not available to be used yet or am I missing something? |
@jackforesightmobile Hope you found your solution, if not in VSCode with Flutter 2.5.3 and Dart 2.14.4 |
@TheGlorySaint Hi, is this lint only work at VSCode? I am using Android Studio and I am also having this issue where I can't seem to have this lint working in my project, meaning there's no warning report to code like this: body: const Center(child: Text('Hello World')), Is there anything else I have to do to have this lint working? Or maybe it's because this PR is being blocked that I can't have this lint working as expected? And here's `my flutter doctor -v`
And yes, I have turn on the lint in my analysis_options.yaml file. I just can't have it... help.... |
@SeasonLeee Did you added
Please try it with Following(longer) Code
|
@SeasonLeee Your Welcome, that my Answer helped you to get it running. How I understood the Trailing Comma is working, it is the length of the Line and a few "Logical" Commas like your first Screenshot. But at the second screenshot the length is fine(for the Trailing Comma) but logically there should be another Trailing Comma after the Text Widget String and I think the Logic isn't far or good enough to Support that feature for that. |
The 2nd condition is that there is only one parameter. Both screens look correct. |
Then the condition is one named parameter, didn't remember exactly :) |
Following-up from flutter/flutter-intellij#2980 (comment), we might consider a rule that enforces the flutter style guide suggestion:
Since a major motivation is to give analyzer something to hang quick-fixes from, we might consider enforcing just the first part and allowing
dartfmt
to handle line-breaks but maybe this would make the rule less useful for flutter repo committers.@Hixie: any thoughts?
/cc @jaumard
The text was updated successfully, but these errors were encountered: