Add the ban-comma-operator rule #3250
Conversation
| default: | ||
| return noCheck(node); | ||
| noCheck(node); | ||
| return; |
There was a problem hiding this comment.
it's not necessary to change the 3 returns above.
There was a problem hiding this comment.
That's true. Changed back.
I changed them for consistency with the others; I don't like that return foo() implies that foo() actually returns a value, rather than has only side effects.
There was a problem hiding this comment.
I introduced that pattern back then thinking this might benefit from tail call optimization. I haven't measured if it actually makes a difference though.
| @@ -109,6 +118,7 @@ function walk(ctx: Lint.WalkContext<void>) { | |||
|
|
|||
| function maybeCallback(cb: (node: ts.Node) => void, node?: ts.Node) { | |||
| if (node !== undefined) { | |||
| return cb(node); | |||
| cb(node); | |||
| return; | |||
There was a problem hiding this comment.
the return here is unnecessary
|
CI is failing. Seems like you branched off an older version of master. |
| @@ -88,6 +88,7 @@ export const rules = { | |||
| // Functionality | |||
| "await-promise": true, | |||
| // "ban": no sensible default | |||
| "ban-comma-operator": true, | |||
There was a problem hiding this comment.
IMO this is also a good candidate for latest.ts
|
Updated, sorry about that. Haven't been watching the CI results closely. I don't really know what I'm doing with git, I rarely use it. :-). For some reason I've gotten my fork into a state where all my PRs start with this same commit, "Merge remote-tracking branch 'palantir/master'", cffa68e. I don't know why. I wish there was a way to have the 'master' branch of my fork just always up to date, since it's clean, instead of having to remember to update before I start a PR branch. And then even when I do remember, this weird commit sticks around with all my branches. |
|
There is actually one valid use case for the comma operator: indirect function calls https://stackoverflow.com/questions/36076794/does-the-comma-operator-influence-the-execution-context-in-javascript/36077031#36077031 (0, eval)("code");I don't know why someone would ever need this in regular code. So my question is if we ignore this completely, add an option to allow it or allow it by default? |
|
Wow, that's crazy. I think I've seen code that does that before but I never knew what it meant. My opinion personally is that that should be rare enough in modern code that a But it's up to y'all. I think it would be pretty easy to do if we wanted to add it later, if someone complains. |
|
I guess you are right. It's rare and that's what disable comments are made for. Thanks @calebegg |
ghost
mentioned this pull request
No description provided.