Issue 29386587: Issue 4993 - Add operator-linebreak rule to our JavaScript style guide (Closed)
|
Created:
March 17, 2017, 10:42 a.m. by kzar Modified:
March 23, 2017, 5:12 a.m. CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 4993 - Add operator-linebreak rule to our JavaScript style guide
See the discussion here https://codereview.adblockplus.org/29384022/
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed Wladimir's feedback #
Total comments: 2
Patch Set 3 : Reworded the sentence about member access operators #MessagesTotal messages: 12
Patch Set 1
https://codereview.adblockplus.org/29386587/diff/29386588/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29386587/diff/29386588/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-operator-linebreak When splitting overlong expressions into multiple lines the joining operators should always be left at the end of the line, before the linebreak.}}</li> "the joining binary operators" please. Also, we need to exclude member access operators here (especially .), these being special.
Patch Set 2 : Addressed Wladimir's feedback https://codereview.adblockplus.org/29386587/diff/29386588/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29386587/diff/29386588/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-operator-linebreak When splitting overlong expressions into multiple lines the joining operators should always be left at the end of the line, before the linebreak.}}</li> On 2017/03/17 11:35:02, Wladimir Palant wrote: > "the joining binary operators" please. Also, we need to exclude member access > operators here (especially .), these being special. Done.
https://codereview.adblockplus.org/29386587/diff/29386645/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29386587/diff/29386645/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-operator-linebreak When splitting overlong expressions into multiple lines the joining binary operators should be left at the end of the line, before the linebreak. The exception being member access operators, such as <code><fix>.</fix></code> which should be placed on the following line.}}</li> I don't feel comfortable advising you about English, but "being" in the last sentence sounds wrong, this being a sentence of its own. Maybe "The exception are member access operators"? Also, I think that there should be a comma before "which."
Patch Set 3 : Reworded the sentence about member access operators https://codereview.adblockplus.org/29386587/diff/29386645/pages/coding-style.... File pages/coding-style.html (right): https://codereview.adblockplus.org/29386587/diff/29386645/pages/coding-style.... pages/coding-style.html:28: <li>{{javascript-operator-linebreak When splitting overlong expressions into multiple lines the joining binary operators should be left at the end of the line, before the linebreak. The exception being member access operators, such as <code><fix>.</fix></code> which should be placed on the following line.}}</li> On 2017/03/17 13:49:25, Wladimir Palant wrote: > I don't feel comfortable advising you about English You should, your English is much better than mine :p > "being" in the last sentence sounds wrong, this being a sentence of its > own. I've had another go, what do you think? > Also, I think that there should be a comma before "which." Yea, I thought that too, but it was kind of confusing having the comma right after the period. To me at least it looked like the operator we were talking about was ".,". I couldn't see another way to reorder the sentence to avoid that, but I'm open to suggestions.
LGTM
What is the issue for this change? The one given in this review's title is apparently wrong. FWIW, when wrapping long lines, I prefer to align operators, like done here: https://hg.adblockplus.org/adblockpluschrome/file/af4fcfc75bb5/include.preloa... Though, I probably wouldn't complain in a codereview if somebody puts the joining operators at the end. And if all of you (Wladimir, Felix and Dave) agree that joining operators should always go at the end, I won't block census here. However, if I'm not the only one who is unsure about this rule, I feel we should rather relax the ESLint configuration, than making our coding style guide unnecessarily strict, here.
On 2017/03/21 11:50:41, Sebastian Noack wrote: > What is the issue for this change? The one given in this review's title is > apparently wrong. Sorry, it's 4993, fixed that now. > FWIW, when wrapping long lines, I prefer to align operators, like done here: > https://hg.adblockplus.org/adblockpluschrome/file/af4fcfc75bb5/include.preloa... > > Though, I probably wouldn't complain in a codereview if somebody puts the > joining operators at the end. And if all of you (Wladimir, Felix and Dave) agree > that joining operators should always go at the end, I won't block census here. > However, if I'm not the only one who is unsure about this rule, I feel we should > rather relax the ESLint configuration, than making our coding style guide > unnecessarily strict, here. Well originally we had the following ESLint rule in our configuration: "operator-linebreak": "error" Which it turned out used the default configuration of something like this: "operator-linebreak": ["error", "after", { "overrides": { "?": "before", ":": "before" } }] This came up in the ESLint review for adblockpluscore, where Wladimir pointed out he expected all such operators to be at the end of the line (as did I) but then we noticed ESLint was expecting "?" and ":" operators to be on the next line. Then Felix pointed out that the Mozilla coding style guide which we inherit from actually specified the following: "Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?:, binary arithmetic operators including +, and member-of operators (in particular the . operator in JavaScript, see the Rationale)." So in order to figure out what we should do, I followed Wladimir's suggestion and ran ESLint with both the operator-linebreak configuration that matched Wladimir and my expectations: "operator-linebreak": ["error", "after"], and the configuration to match Mozilla's coding style: "operator-linebreak": ["error", "before", { "overrides": { "||": "after", "&&": "after" } }] I tested against the three repositories _before_ all the ESLint changes and the result was clearly in favour of the the first rule. Have a look at the review linked in the description here for the numbers. Obviously post-ESLint changes (adblockplusui and adblockpluscore already landed) the first rule has no violations and the second has even more. So then we agreed to go with the first rule, so long as we updated our coding style guide to match. Hence this review. The corresponding ESLint changes already landed, and adblockpluscore and adblockplusui already conform. After this patch lands and the ESLint changes for adblockpluschrome land we're done. I'm hoping we can just go with this as is, since I'm going (not so) slowly crazy trying to get this ESLint stuff finished.
On 2017/03/22 05:49:06, kzar wrote: > On 2017/03/21 11:50:41, Sebastian Noack wrote: > > What is the issue for this change? The one given in this review's title is > > apparently wrong. > > Sorry, it's 4993, fixed that now. > > > FWIW, when wrapping long lines, I prefer to align operators, like done here: > > > https://hg.adblockplus.org/adblockpluschrome/file/af4fcfc75bb5/include.preloa... > > > > Though, I probably wouldn't complain in a codereview if somebody puts the > > joining operators at the end. And if all of you (Wladimir, Felix and Dave) > agree > > that joining operators should always go at the end, I won't block census here. > > However, if I'm not the only one who is unsure about this rule, I feel we > should > > rather relax the ESLint configuration, than making our coding style guide > > unnecessarily strict, here. > > Well originally we had the following ESLint rule in our configuration: > > "operator-linebreak": "error" > > Which it turned out used the default configuration of something like this: > > "operator-linebreak": ["error", "after", { "overrides": { "?": "before", ":": > "before" } }] > > This came up in the ESLint review for adblockpluscore, where Wladimir pointed > out he expected all such operators to be at the end of the line (as did I) but > then we noticed ESLint was expecting "?" and ":" operators to be on the next > line. > > Then Felix pointed out that the Mozilla coding style guide which we inherit from > actually specified the following: > > "Overlong expressions not joined by && and || should break so the operator > starts on the second line and starts in the same column as the start of the > expression in the first line. This applies to ?:, binary arithmetic operators > including +, and member-of operators (in particular the . operator in > JavaScript, see the Rationale)." > > So in order to figure out what we should do, I followed Wladimir's suggestion > and ran ESLint with both the operator-linebreak configuration that matched > Wladimir and my expectations: > > "operator-linebreak": ["error", "after"], > > and the configuration to match Mozilla's coding style: > > "operator-linebreak": ["error", "before", { "overrides": { "||": "after", > "&&": "after" } }] > > I tested against the three repositories _before_ all the ESLint changes and the > result was clearly in favour of the the first rule. Have a look at the review > linked in the description here for the numbers. Obviously post-ESLint changes > (adblockplusui and adblockpluscore already landed) the first rule has no > violations and the second has even more. > > So then we agreed to go with the first rule, so long as we updated our coding > style guide to match. Hence this review. > > The corresponding ESLint changes already landed, and adblockpluscore and > adblockplusui already conform. After this patch lands and the ESLint changes for > adblockpluschrome land we're done. I'm hoping we can just go with this as is, > since I'm going (not so) slowly crazy trying to get this ESLint stuff finished. I see, thanks for the detailed explanation. It seems my preferred style mostly matches Mozilla's coding practices (except that I don't see why they make and exception for && and ||). But either way, standardizing this doesn't seem worth it to me, since I neither can think of any potential bugs prevented by this rule, neither do I remember any controversy in codereviews resulting from a lack of this rule. So I'd vote for just turning the operator-linebreak rule off. Let's see what Felix says.
On 2017/03/22 06:49:02, Sebastian Noack wrote: > On 2017/03/22 05:49:06, kzar wrote: > > On 2017/03/21 11:50:41, Sebastian Noack wrote: > > > What is the issue for this change? The one given in this review's title is > > > apparently wrong. > > > > Sorry, it's 4993, fixed that now. > > > > > FWIW, when wrapping long lines, I prefer to align operators, like done here: > > > > > > https://hg.adblockplus.org/adblockpluschrome/file/af4fcfc75bb5/include.preloa... > > > > > > Though, I probably wouldn't complain in a codereview if somebody puts the > > > joining operators at the end. And if all of you (Wladimir, Felix and Dave) > > agree > > > that joining operators should always go at the end, I won't block census > here. > > > However, if I'm not the only one who is unsure about this rule, I feel we > > should > > > rather relax the ESLint configuration, than making our coding style guide > > > unnecessarily strict, here. > > > > Well originally we had the following ESLint rule in our configuration: > > > > "operator-linebreak": "error" > > > > Which it turned out used the default configuration of something like this: > > > > "operator-linebreak": ["error", "after", { "overrides": { "?": "before", > ":": > > "before" } }] > > > > This came up in the ESLint review for adblockpluscore, where Wladimir pointed > > out he expected all such operators to be at the end of the line (as did I) but > > then we noticed ESLint was expecting "?" and ":" operators to be on the next > > line. > > > > Then Felix pointed out that the Mozilla coding style guide which we inherit > from > > actually specified the following: > > > > "Overlong expressions not joined by && and || should break so the operator > > starts on the second line and starts in the same column as the start of the > > expression in the first line. This applies to ?:, binary arithmetic > operators > > including +, and member-of operators (in particular the . operator in > > JavaScript, see the Rationale)." > > > > So in order to figure out what we should do, I followed Wladimir's suggestion > > and ran ESLint with both the operator-linebreak configuration that matched > > Wladimir and my expectations: > > > > "operator-linebreak": ["error", "after"], > > > > and the configuration to match Mozilla's coding style: > > > > "operator-linebreak": ["error", "before", { "overrides": { "||": "after", > > "&&": "after" } }] > > > > I tested against the three repositories _before_ all the ESLint changes and > the > > result was clearly in favour of the the first rule. Have a look at the review > > linked in the description here for the numbers. Obviously post-ESLint changes > > (adblockplusui and adblockpluscore already landed) the first rule has no > > violations and the second has even more. > > > > So then we agreed to go with the first rule, so long as we updated our coding > > style guide to match. Hence this review. > > > > The corresponding ESLint changes already landed, and adblockpluscore and > > adblockplusui already conform. After this patch lands and the ESLint changes > for > > adblockpluschrome land we're done. I'm hoping we can just go with this as is, > > since I'm going (not so) slowly crazy trying to get this ESLint stuff > finished. > > I see, thanks for the detailed explanation. It seems my preferred style mostly > matches Mozilla's coding practices (except that I don't see why they make and > exception for && and ||). But either way, standardizing this doesn't seem worth > it to me, since I neither can think of any potential bugs prevented by this > rule, neither do I remember any controversy in codereviews resulting from a lack > of this rule. So I'd vote for just turning the operator-linebreak rule off. > Let's see what Felix says. I personally prefer sticking to the Mozilla style as well and I agree that it's no massive problem if we're inconsistent here. However, I do think it's pretty confusing if we override the Mozilla style guide specifically to remove that rule (rather than change it). Given how we almost exclusively seem to go for wrapping after all operators, I'd say let's just standardise that and call it a day. One thing less to argue about.
LGTM for the actual change, BTW.
On 2017/03/22 20:17:55, Felix Dahlke wrote: > On 2017/03/22 06:49:02, Sebastian Noack wrote: > > On 2017/03/22 05:49:06, kzar wrote: > > > On 2017/03/21 11:50:41, Sebastian Noack wrote: > > > > What is the issue for this change? The one given in this review's title is > > > > apparently wrong. > > > > > > Sorry, it's 4993, fixed that now. > > > > > > > FWIW, when wrapping long lines, I prefer to align operators, like done > here: > > > > > > > > > > https://hg.adblockplus.org/adblockpluschrome/file/af4fcfc75bb5/include.preloa... > > > > > > > > Though, I probably wouldn't complain in a codereview if somebody puts the > > > > joining operators at the end. And if all of you (Wladimir, Felix and Dave) > > > agree > > > > that joining operators should always go at the end, I won't block census > > here. > > > > However, if I'm not the only one who is unsure about this rule, I feel we > > > should > > > > rather relax the ESLint configuration, than making our coding style guide > > > > unnecessarily strict, here. > > > > > > Well originally we had the following ESLint rule in our configuration: > > > > > > "operator-linebreak": "error" > > > > > > Which it turned out used the default configuration of something like this: > > > > > > "operator-linebreak": ["error", "after", { "overrides": { "?": "before", > > ":": > > > "before" } }] > > > > > > This came up in the ESLint review for adblockpluscore, where Wladimir > pointed > > > out he expected all such operators to be at the end of the line (as did I) > but > > > then we noticed ESLint was expecting "?" and ":" operators to be on the next > > > line. > > > > > > Then Felix pointed out that the Mozilla coding style guide which we inherit > > from > > > actually specified the following: > > > > > > "Overlong expressions not joined by && and || should break so the operator > > > starts on the second line and starts in the same column as the start of > the > > > expression in the first line. This applies to ?:, binary arithmetic > > operators > > > including +, and member-of operators (in particular the . operator in > > > JavaScript, see the Rationale)." > > > > > > So in order to figure out what we should do, I followed Wladimir's > suggestion > > > and ran ESLint with both the operator-linebreak configuration that matched > > > Wladimir and my expectations: > > > > > > "operator-linebreak": ["error", "after"], > > > > > > and the configuration to match Mozilla's coding style: > > > > > > "operator-linebreak": ["error", "before", { "overrides": { "||": "after", > > > "&&": "after" } }] > > > > > > I tested against the three repositories _before_ all the ESLint changes and > > the > > > result was clearly in favour of the the first rule. Have a look at the > review > > > linked in the description here for the numbers. Obviously post-ESLint > changes > > > (adblockplusui and adblockpluscore already landed) the first rule has no > > > violations and the second has even more. > > > > > > So then we agreed to go with the first rule, so long as we updated our > coding > > > style guide to match. Hence this review. > > > > > > The corresponding ESLint changes already landed, and adblockpluscore and > > > adblockplusui already conform. After this patch lands and the ESLint changes > > for > > > adblockpluschrome land we're done. I'm hoping we can just go with this as > is, > > > since I'm going (not so) slowly crazy trying to get this ESLint stuff > > finished. > > > > I see, thanks for the detailed explanation. It seems my preferred style mostly > > matches Mozilla's coding practices (except that I don't see why they make and > > exception for && and ||). But either way, standardizing this doesn't seem > worth > > it to me, since I neither can think of any potential bugs prevented by this > > rule, neither do I remember any controversy in codereviews resulting from a > lack > > of this rule. So I'd vote for just turning the operator-linebreak rule off. > > Let's see what Felix says. > > I personally prefer sticking to the Mozilla style as well and I agree that it's > no massive problem if we're inconsistent here. > > However, I do think it's pretty confusing if we override the Mozilla style guide > specifically to remove that rule (rather than change it). Given how we almost > exclusively seem to go for wrapping after all operators, I'd say let's just > standardise that and call it a day. One thing less to argue about. You made a point, if we leave our coding style guide as is, we'd have to follow the Mozilla style consistently. So since I guess nobody of us wants that, we have to override it here anyway. But overriding a rule inherited from Mozilla, just to remove it, seems confusing. So we can better standardize what we are doing for the most part already. Fair enough, I guess. LGTM. 
|
