Issue 29377779: Issue 3692 - Add arrow-parens ESLint rule (Closed)
DescriptionIssue 3692 - Add arrow-parens ESLint rule
This rule requires that the parenthesis around the arguments for arrow functions are always used, even when not required.
So for example `(foo) => foo` is OK, but not `foo => foo`.
Notice that this rule doe not care about the braces around the function's body.
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 12
Patch Set 1 This came up in the related adblockplusui review. Apparently Thomas and some of the Flattr people prefer using the argument parenthesis consistently for arrow functions. Personally I don't care either way, so long as we agree it now once and for all!
On 2017/03/02 04:28:01, kzar wrote: > Patch Set 1 > > This came up in the related adblockplusui review. Apparently Thomas and some of > the Flattr people prefer using the argument parenthesis consistently for arrow > functions. > > Personally I don't care either way, so long as we agree it now once and for all! For what it's worth I would vote in favor. I don't like the inconsistency of `() => foo` vs `foo => foo` vs `(foo, bar) => foo` and would prefer to use parenthesis in all three cases.
https://codereview.adblockplus.org/29377779/diff/29377780/eslint-config-eyeo/... File eslint-config-eyeo/index.js (right): https://codereview.adblockplus.org/29377779/diff/29377780/eslint-config-eyeo/... eslint-config-eyeo/index.js:29: "arrow-parens": "error", Going through adblockplus and adblockpluscore repositories, the use of parentheses for single-parameter arrow functions is inconsistent with a tendency towards no parentheses. I think this might be the result of my personal preferences changing over time. In adblockpluschrome no parentheses are being used very consistently. I don't really see a case for enforcing parentheses from our existing codebase. I also don't see how it improves readability or prevents bugs. So this is clearly NOT LGTM to me. My preferred solution would be to leave this rule out and keep allowing personal preferences here since they don't affect anything. If we want this to be uniform however, I'd vote for the "as-needed" setting. Maybe Thomas can help me understand how this is going to improve the quality of our code.
On 2017/03/02 10:03:29, Wladimir Palant wrote: > My preferred solution would be to leave this rule out and keep allowing > personal preferences here since they don't affect anything. If > we want this to be uniform however, I'd vote for the "as-needed" setting. Well I think we should decide one way or the other now and be done with it. That's kind of the point of using ESLint, avoiding having these discussions constantly. I'm happy to go with either "as-needed" or "always", as long as we decide. My personal preference is "as-needed" but I don't mind too much. What do you think Felix, do you prefer `foo => foo` or `(foo) => foo`?
> Maybe Thomas can help me understand how this is going to improve the quality of our code. This is more about making it easier to change code during development. It's quite a hassle unnecessarily having to remove/add brackets whenever you want to change the number of arguments in a function signature (similar to having to add/remove braces depending on how many lines an if-statement has but that's a different story). From a personal perspective, my other argument for it is quite similar to the one I had against the "brackets always on new line" rule: There's only one way how it can be made consistent so why not embrace it wherever we can? Generally, I'm not against using "as-needed" in the global eslint-config-eyeo because we could still leave it up to the individual repositories to extend the config.
On 2017/03/02 10:20:41, kzar wrote: > On 2017/03/02 10:03:29, Wladimir Palant wrote: > > My preferred solution would be to leave this rule out and keep allowing > > personal preferences here since they don't affect anything. If > > we want this to be uniform however, I'd vote for the "as-needed" setting. > > Well I think we should decide one way or the other now and be done with it. > That's kind of the point of using ESLint, avoiding having these discussions > constantly. > > I'm happy to go with either "as-needed" or "always", as long as we decide. My > personal preference is "as-needed" but I don't mind too much. > > What do you think Felix, do you prefer `foo => foo` or `(foo) => foo`? My personal preference is the former, but I don't mind the latter either. I see it like Wladimir: Why enforce such a thing that doesn't really affect readability? We don't enforce braces around single line condition/loop bodies either. I doubt this'll lead to many discussions, if something is not specified in our coding style or our ESLint config, it's up for the author (not reviewer) to decide, simple enough. We can always add this rule later if we can demonstrate that not standardising it leads to problems. On 2017/03/02 11:15:57, Thomas Greiner wrote: > > Maybe Thomas can help me understand how this is going to improve the quality > of > our code. > > This is more about making it easier to change code during development. It's > quite a hassle unnecessarily having to remove/add brackets whenever you want to > change the number of arguments in a function signature (similar to having to > add/remove braces depending on how many lines an if-statement has but that's a > different story). See above, kind of the same thing in my mind. So we're being consistent by approaching these issues the same way :) > Generally, I'm not against using "as-needed" in the global eslint-config-eyeo > because we could still leave it up to the individual repositories to extend the > config. No I think we should absolutely avoid that: We need to have one coding style for our entire code base, not one per project...
On 2017/03/02 11:43:13, Felix Dahlke wrote: > My personal preference is the former, but I don't mind the latter either. I see > it like Wladimir: Why enforce such a thing that doesn't really affect > readability? To avoid ever having this discussion again. If we decide it now then we're done. > We don't enforce braces around single line condition/loop bodies either. Actually our ESLint configuration does, it forbids braces for those.
On 2017/03/02 11:43:13, Felix Dahlke wrote: > On 2017/03/02 11:15:57, Thomas Greiner wrote: > > > Maybe Thomas can help me understand how this is going to improve the quality > > of > > our code. > > > > This is more about making it easier to change code during development. It's > > quite a hassle unnecessarily having to remove/add brackets whenever you want > to > > change the number of arguments in a function signature (similar to having to > > add/remove braces depending on how many lines an if-statement has but that's a > > different story). > > See above, kind of the same thing in my mind. So we're being consistent by > approaching these issues the same way :) Except that in regards to braces we don't leave it to the author. Or were you referring to something different? > No I think we should absolutely avoid that: We need to have one coding style for our entire code base, not one per project... I was only referring to making it stricter, not looser.
On 2017/03/02 11:56:57, Thomas Greiner wrote: > Except that in regards to braces we don't leave it to the author. Seeing https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j..., we should probably reconsider it. ESLint being overly dogmatic about this isn't really helping.
And another example where being strict about curly braces caused ridiculous results: https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener....
On 2017/03/02 12:56:34, Wladimir Palant wrote: > And another example where being strict about curly braces caused ridiculous > results: > https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener.... That stuff is not really related to this review but: I basically argued that we should leave both of these edge cases (the one discussed here and the single line condition/loop bodies) up to the author. Is that not the case for single line condition/loop bodies now? I definitely don't think we should _force_ them! Just leave it up to the author. IMHO, we should always when in doubt _not_ enforce things with ESLint. We can always add more rules later, but I fear if we want to have a mass discussion on every single aspect of the language now it's going to take forever to land this. It seems like an amazing first step to me to enforce everything we already agree on - that's a lot, and that'll make it a lot easier to contribute to our code base while retaining ones sanity. We can tackle weird edge cases like this one later, when we stumble across them as a real problem, can't we?
Message was sent while issue was closed.
OK I've closed this issue since mostly people seem against adding the arrow-parens rule to our main configuration. Like Thomas points out we can always add that rule to repositories like adblockplusui where a certain style is enforced during reviews. That will still avoid time wasting in codereviews without being too dogmatic elsewhere. Furthermore it seems from the discussion most people are against how our curly rule enforced brackets for single line blocks. I've just looked into it and can't find a way to enforce brackets for multi-line blocks without enforcing them for single line ones. So I've opened a review to just remove that controversial rule. https://codereview.adblockplus.org/29377837/ I agree with Felix, no point arguing the edge cases since we already agree on most rules now. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
