Discussion:
[chromium-dev] Re: Cr JS Style remove redundant entry for braces on if statements
s***@chromium.org
2018-11-16 01:11:10 UTC
Permalink
Sorry for reviving this, but there's still an entry in our webUI styleguide
regarding if-statement braces, and people are still confused about whether
things need braces.

According to google styleguide:

if (...) {
...one line;
} else {
...one line;
}

if (...) {
......... one line;
}

will all need braces. the only case that would not need braces is if the
if-statement AND the one-liner both fit on the same line:

if (...) one liner;

It seems like the discussion in this thread had converged on "lets remove
chrome's styleguide on if-statements, and just follow google's styleguide"
but this hasn't happened yet. If there's no further concerns I'll be making
a CL to update our styleguide and also add an eslint rule to enforce this
in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then clauses.
Some feel they should be mandatory. Nevertheless, as a code reviewer, don't
force people to add/remove them just because it conflicts with your own
personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++ style
guide (that *kinda* applied to JS) about preferring less curlies for
simple conditionals, but I don't see it any more. Maybe I was imagining it
or it has disappeared (and maybe is still in effect from a different style
guide, i.e. Google?). Either way, I don't see a strong case for asking
folks to NOT use curlies where they like them (other than maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
I've always thought the "omit curlies" rule was a bit unsafe (see: goto
fail; bug). I followed "omit curlies" in reviews/my code because it's What
I Was Told. Now that the Google JS style guide has a clear opinion on this
(and most of the folks that hated curlies have moved on ;)), I think it'd
be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from the
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a "redundant"
no-op change to remove this style guidance (and fall through to the Google
style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements in the style
guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the intent)
publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org.
Michael Giuffrida
2018-11-16 02:46:04 UTC
Permalink
We shouldn't be deciding whether to change style guidelines in a CL, let
alone a year-old one.
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if the
if (...) one liner;
It seems like the discussion in this thread had converged on "lets remove
chrome's styleguide on if-statements, and just follow google's styleguide"
but this hasn't happened yet. If there's no further concerns I'll be making
a CL to update our styleguide and also add an eslint rule to enforce this
in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++ style
guide (that *kinda* applied to JS) about preferring less curlies for
simple conditionals, but I don't see it any more. Maybe I was imagining it
or it has disappeared (and maybe is still in effect from a different style
guide, i.e. Google?). Either way, I don't see a strong case for asking
folks to NOT use curlies where they like them (other than maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
I've always thought the "omit curlies" rule was a bit unsafe (see: goto
fail; bug). I followed "omit curlies" in reviews/my code because it's What
I Was Told. Now that the Google JS style guide has a clear opinion on this
(and most of the folks that hated curlies have moved on ;)), I think it'd
be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from the
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a "redundant"
no-op change to remove this style guidance (and fall through to the Google
style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements in
the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the intent)
publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACi5S_2VDxHz7o9TNHFP7UBBVr-2Th0ukheXMwH1co2KmPBx2Q%40mail.gmail.com.
dpapad
2018-11-16 02:55:40 UTC
Permalink
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL, let
alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would not be
debating within the CL itself.

FWIW, during reviews I've frequently had to

- explain what a "single line" statement is
- whether an if..else with single lines inside each branch qualifies
for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the "else" (or
vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.


Regardless of the outcome of this revived discussion, not having to do this
would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if the
if (...) one liner;
It seems like the discussion in this thread had converged on "lets remove
chrome's styleguide on if-statements, and just follow google's styleguide"
but this hasn't happened yet. If there's no further concerns I'll be making
a CL to update our styleguide and also add an eslint rule to enforce this
in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++ style
guide (that *kinda* applied to JS) about preferring less curlies for
simple conditionals, but I don't see it any more. Maybe I was imagining it
or it has disappeared (and maybe is still in effect from a different style
guide, i.e. Google?). Either way, I don't see a strong case for asking
folks to NOT use curlies where they like them (other than maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
I've always thought the "omit curlies" rule was a bit unsafe (see: goto
fail; bug). I followed "omit curlies" in reviews/my code because it's What
I Was Told. Now that the Google JS style guide has a clear opinion on this
(and most of the folks that hated curlies have moved on ;)), I think it'd
be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from the
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a "redundant"
no-op change to remove this style guidance (and fall through to the Google
style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements in
the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the intent)
publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/049dd445-3fd2-4163-8779-1cce823fd947%40chromium.org.
Michael Giuffrida
2018-11-16 03:09:43 UTC
Permalink
Whoops. I was on mobile and thought I was replying to a CL rather than
chromium-dev itself. Apologies!

I still prefer omitting braces, but not enough to block us from aligning
with Google style.
Post by dpapad
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL, let
alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would not be
debating within the CL itself.
FWIW, during reviews I've frequently had to
- explain what a "single line" statement is
- whether an if..else with single lines inside each branch qualifies
for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the "else"
(or vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.
Regardless of the outcome of this revived discussion, not having to do
this would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if the
if (...) one liner;
It seems like the discussion in this thread had converged on "lets
remove chrome's styleguide on if-statements, and just follow google's
styleguide" but this hasn't happened yet. If there's no further concerns
I'll be making a CL to update our styleguide and also add an eslint rule to
enforce this in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++ style
guide (that *kinda* applied to JS) about preferring less curlies for
simple conditionals, but I don't see it any more. Maybe I was imagining it
or it has disappeared (and maybe is still in effect from a different style
guide, i.e. Google?). Either way, I don't see a strong case for asking
folks to NOT use curlies where they like them (other than maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
goto fail; bug). I followed "omit curlies" in reviews/my code because it's
What I Was Told. Now that the Google JS style guide has a clear opinion on
this (and most of the folks that hated curlies have moved on ;)), I think
it'd be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from the
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a "redundant"
no-op change to remove this style guidance (and fall through to the Google
style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements in
the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the intent)
publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/049dd445-3fd2-4163-8779-1cce823fd947%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/049dd445-3fd2-4163-8779-1cce823fd947%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACi5S_3MMQFKkL_5x31QWBNySxB%2BBXFVhqybxaCQHVP2to2rOw%40mail.gmail.com.
Tommy C. Li
2018-11-16 18:54:55 UTC
Permalink
I would also support conforming to the Google JavaScript style guide, which
requires braces everywhere.

if (foo) {
bar();
}

Often when I omit them, I regret it when I need to add more statements to
the body of the control structure. And it also changes the git-blame on the
conditional predicate too.

It's not a huge deal of course - I just don't think we have a strong enough
justification to deviate from the general style guide.
Post by dpapad
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL, let
alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would not be
debating within the CL itself.
FWIW, during reviews I've frequently had to
- explain what a "single line" statement is
- whether an if..else with single lines inside each branch qualifies
for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the "else"
(or vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.
Regardless of the outcome of this revived discussion, not having to do
this would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if the
if (...) one liner;
It seems like the discussion in this thread had converged on "lets
remove chrome's styleguide on if-statements, and just follow google's
styleguide" but this hasn't happened yet. If there's no further concerns
I'll be making a CL to update our styleguide and also add an eslint rule to
enforce this in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++ style
guide (that *kinda* applied to JS) about preferring less curlies for
simple conditionals, but I don't see it any more. Maybe I was imagining it
or it has disappeared (and maybe is still in effect from a different style
guide, i.e. Google?). Either way, I don't see a strong case for asking
folks to NOT use curlies where they like them (other than maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
goto fail; bug). I followed "omit curlies" in reviews/my code because it's
What I Was Told. Now that the Google JS style guide has a clear opinion on
this (and most of the folks that hated curlies have moved on ;)), I think
it'd be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from the
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a "redundant"
no-op change to remove this style guidance (and fall through to the Google
style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements in
the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the intent)
publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1b14544c-e416-4f96-804c-e1da1d5f1147%40chromium.org.
Dan Beam
2018-11-27 20:24:06 UTC
Permalink
Thanks everyone for the additional feedback! Seems like there's some in
favor of removing, and no strong votes against.
If no other objections occur I'll look to create a CL to change the style
guide and add a lint rule early next week (12/3~, after m72 branch cut).
I don't have strong feelings either way, but do we plan on updating
existing, non-conforming code? (As in: all the places we omit curlies?)

-- Dan
Post by Tommy C. Li
I would also support conforming to the Google JavaScript style guide,
which requires braces everywhere.
if (foo) {
bar();
}
Often when I omit them, I regret it when I need to add more statements to
the body of the control structure. And it also changes the git-blame on the
conditional predicate too.
It's not a huge deal of course - I just don't think we have a strong
enough justification to deviate from the general style guide.
Post by dpapad
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL,
let alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would not
be debating within the CL itself.
FWIW, during reviews I've frequently had to
- explain what a "single line" statement is
- whether an if..else with single lines inside each branch
qualifies for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the "else"
(or vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.
Regardless of the outcome of this revived discussion, not having to do
this would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if
if (...) one liner;
It seems like the discussion in this thread had converged on "lets
remove chrome's styleguide on if-statements, and just follow google's
styleguide" but this hasn't happened yet. If there's no further concerns
I'll be making a CL to update our styleguide and also add an eslint rule to
enforce this in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++
style guide (that *kinda* applied to JS) about preferring less
curlies for simple conditionals, but I don't see it any more. Maybe I was
imagining it or it has disappeared (and maybe is still in effect from a
different style guide, i.e. Google?). Either way, I don't see a strong
case for asking folks to NOT use curlies where they like them (other than
maybe precedence).
My preference would be to remove the Chromium styleguide variation
(based on this interpretation), and adhere to the Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
goto fail; bug). I followed "omit curlies" in reviews/my code because it's
What I Was Told. Now that the Google JS style guide has a clear opinion on
this (and most of the folks that hated curlies have moved on ;)), I think
it'd be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides Google's
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a
"redundant" no-op change to remove this style guidance (and fall through to
the Google style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
On Fri, Jul 28, 2017 at 4:02 PM, Dave Schuyler <
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements
in the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the
intent) publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_QoZAF4rm7u5CFmKtq-_HfsSi2tzX3nFqJ9xxt_oUoncQQ%40mail.gmail.com.
Demetrios Papadopoulos
2018-11-27 22:16:25 UTC
Permalink
My plan was to first add an eslint rule and then fix all the warnings it
give me, so yes I think it would cover all the places we omit curlies.
Adding a new ESLint to the PRESUBMIT does not automatically give any
warnings. Only files that are affected in a CL are checked against ESLint.

In order to achieve this you would need to manually trigger ESLint an all
related files.
Post by Dan Beam
Thanks everyone for the additional feedback! Seems like there's some in
favor of removing, and no strong votes against.
If no other objections occur I'll look to create a CL to change the
style guide and add a lint rule early next week (12/3~, after m72 branch
cut).
I don't have strong feelings either way, but do we plan on updating
existing, non-conforming code? (As in: all the places we omit curlies?)
-- Dan
Post by Tommy C. Li
I would also support conforming to the Google JavaScript style guide,
which requires braces everywhere.
if (foo) {
bar();
}
Often when I omit them, I regret it when I need to add more statements
to the body of the control structure. And it also changes the git-blame on
the conditional predicate too.
It's not a huge deal of course - I just don't think we have a strong
enough justification to deviate from the general style guide.
Post by dpapad
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL,
let alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would not
be debating within the CL itself.
FWIW, during reviews I've frequently had to
- explain what a "single line" statement is
- whether an if..else with single lines inside each branch
qualifies for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the
"else" (or vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.
Regardless of the outcome of this revived discussion, not having to do
this would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is if
if (...) one liner;
It seems like the discussion in this thread had converged on "lets
remove chrome's styleguide on if-statements, and just follow google's
styleguide" but this hasn't happened yet. If there's no further concerns
I'll be making a CL to update our styleguide and also add an eslint rule to
enforce this in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++
style guide (that *kinda* applied to JS) about preferring less
curlies for simple conditionals, but I don't see it any more. Maybe I was
imagining it or it has disappeared (and maybe is still in effect from a
different style guide, i.e. Google?). Either way, I don't see a strong
case for asking folks to NOT use curlies where they like them (other than
maybe precedence).
My preference would be to remove the Chromium styleguide
variation (based on this interpretation), and adhere to the
Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
goto fail; bug). I followed "omit curlies" in reviews/my code because it's
What I Was Told. Now that the Google JS style guide has a clear opinion on
this (and most of the folks that hated curlies have moved on ;)), I think
it'd be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if possible.
Also note: clang-format -style=Chromium currently overrides
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a
"redundant" no-op change to remove this style guidance (and fall through to
the Google style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
On Fri, Jul 28, 2017 at 4:02 PM, Dave Schuyler <
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line if-statements
in the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the
intent) publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAPUSrA0eCMjfvmKxD1CahZS5fwrzp1HRaGBFjj5aeOA-TiE_LA%40mail.gmail.com.
s***@chromium.org
2018-12-03 19:28:43 UTC
Permalink
Ack, will do.
Post by Demetrios Papadopoulos
My plan was to first add an eslint rule and then fix all the warnings it
give me, so yes I think it would cover all the places we omit curlies.
Adding a new ESLint to the PRESUBMIT does not automatically give any
warnings. Only files that are affected in a CL are checked against ESLint.
In order to achieve this you would need to manually trigger ESLint an all
related files.
Post by Dan Beam
Thanks everyone for the additional feedback! Seems like there's some in
favor of removing, and no strong votes against.
If no other objections occur I'll look to create a CL to change the
style guide and add a lint rule early next week (12/3~, after m72 branch
cut).
I don't have strong feelings either way, but do we plan on updating
existing, non-conforming code? (As in: all the places we omit curlies?)
-- Dan
Post by Tommy C. Li
I would also support conforming to the Google JavaScript style guide,
which requires braces everywhere.
if (foo) {
bar();
}
Often when I omit them, I regret it when I need to add more statements
to the body of the control structure. And it also changes the git-blame on
the conditional predicate too.
It's not a huge deal of course - I just don't think we have a strong
enough justification to deviate from the general style guide.
Post by dpapad
Post by Michael Giuffrida
We shouldn't be deciding whether to change style guidelines in a CL,
let alone a year-old one.
IIUC, the CL would be the outcome of the discussion here. We would
not be debating within the CL itself.
FWIW, during reviews I've frequently had to
- explain what a "single line" statement is
- whether an if..else with single lines inside each branch
qualifies for omitting the braces.
- Whether it is OK to omit braces in the "if" but not in the
"else" (or vice versa)
- link to the Google styleguide
- link to the Chromium's override of that particular rule.
Regardless of the outcome of this revived discussion, not having to
do this would save time for everyone involved. Primarily I am in favor of a
solution that can be automated by leveraging
https://eslint.org/docs/rules/curly (or some other ESLint rule).
Secondarily, I don't feel too strongly about allowing no braces (i like
omitting them, but I can live without them), so removing our override, and
aligning with the Google styleguide seems to make things simpler.
Post by Michael Giuffrida
Post by s***@chromium.org
Sorry for reviving this, but there's still an entry in our webUI
styleguide regarding if-statement braces, and people are still confused
about whether things need braces.
if (...) {
...one line;
} else {
...one line;
}
if (...) {
......... one line;
}
will all need braces. the only case that would not need braces is
if (...) one liner;
It seems like the discussion in this thread had converged on "lets
remove chrome's styleguide on if-statements, and just follow google's
styleguide" but this hasn't happened yet. If there's no further concerns
I'll be making a CL to update our styleguide and also add an eslint rule to
enforce this in the future.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eIEY-0UKQxw/O_wiacC9CAAJ
tl;dr: In C++ code, braces are optional for single-statement then
clauses. Some feel they should be mandatory. Nevertheless, as a code
reviewer, don't force people to add/remove them just because it conflicts
with your own personal preference.
Based on the fact that there are quite a few occurrences of
if (foo)
bar();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/prefs/prefs.js?l=274-275>,
and
if (foo)
bar();
else
baz();
for example here
<https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/controls/settings_slider.js?type=cs&q=else+file:%5Esrc/chrome/browser/resources/settings/+package:%5Echromium$&l=68-71>,
in WebUI JS code, as well as stylistic comments I've received in past code
reviews, I believe that a single-line if statement is interpreted as "*single-line
|if| block without counting the |if| line, and single line |else/else if|
block without counting the |else/else if| line*".
This is my interpretation as well.
I believe there used to be specific guidance in the Chromium C++
style guide (that *kinda* applied to JS) about preferring less
curlies for simple conditionals, but I don't see it any more. Maybe I was
imagining it or it has disappeared (and maybe is still in effect from a
different style guide, i.e. Google?). Either way, I don't see a strong
case for asking folks to NOT use curlies where they like them (other than
maybe precedence).
My preference would be to remove the Chromium styleguide
variation (based on this interpretation), and adhere to the
Google styleguide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>.
In other words the snippets above would be invalid, and the valid versions
would be
if (foo) bar();
if (foo) {
bar();
}
if (foo) {
bar();
} else {
baz();
}
^ This seems fine to me.
I've always thought the "omit curlies" rule was a bit unsafe
(see: goto fail; bug). I followed "omit curlies" in reviews/my code
because it's What I Was Told. Now that the Google JS style guide has a
clear opinion on this (and most of the folks that hated curlies have moved
on ;)), I think it'd be fine to adopt your proposal above.
It'd obviously be nice to update existing, non-compliant code if
possible.
Also note: clang-format -style=Chromium currently overrides
https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L714
-- Dan
Thanks,
Demetrios
Depending on how "single-line if statement" is interpreted from
- Omit curly braces for single-line if statements
may result in a change in the styling. So this may be a
"redundant" no-op change to remove this style guidance (and fall through to
the Google style guide) or it may be a change.
The debate is a bit involved. Please ask if you'd like details.
On Fri, Jul 28, 2017 at 4:02 PM, Dave Schuyler <
[Audience: developers writing JavaScript code in Chromium]
Let's* remove the redundant entry for single line
if-statements in the style guide
<https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#Style>.
- Omit curly braces for single-line if statements.
The Google style guide
<https://google.github.io/styleguide/jsguide.html#formatting-braces>
now has the same guidance.
*Err, I'll do it. Just trying to saying it (announcing the
intent) publicly.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/1e400180-07f6-4034-90bd-5cd59f719fe3%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the
Google Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANNW_Qp%3D0iVS7PEVMr_ZvgMRL_UnfdZx-wRN-ZKo7QJmJJ_XAA%40mail.gmail.com?utm_medium=email&utm_source=footer>
.
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/9db33cf4-8fe5-4f71-a07f-46b839394fc9%40chromium.org?utm_medium=email&utm_source=footer>
.
--
--
Chromium Developers mailing list: chromium-***@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+***@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d9d739c8-6fd2-4020-a11f-f1302910f047%40chromium.org.
Loading...