Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

fix no unnecessary space before parameter from arrow-parens fixer #3618

Merged
merged 5 commits into from Jan 2, 2018

Conversation

mateuszwitkowski
Copy link
Contributor

PR checklist

Overview of change:

Fixer for ban-single-arg-parens doesn't add unnecessary whitespace before argument if parent node is a call expresion.

CHANGELOG.md entry:

[bugfix] no unnecessary whitespace before argument in callback functions fixed with arrow-parens fixer

@@ -38,3 +40,4 @@ let foo = bar => bar;

let foo = async bar => bar;

arr.map(a => b);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will probably fail for foo(async (a) => b) by fixing it to foo(asyca => b)

@@ -72,8 +72,10 @@ function walk(ctx: Lint.WalkContext<Options>) {
}
} else if (ctx.options.banSingleArgParens) {
const closeParen = getChildOfKind(node, ts.SyntaxKind.CloseParenToken)!;
const parentNode = node.parent!;
const replaceValue = ts.isCallExpression(parentNode) ? "" : " ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be limited to CallExpressions. This is also relevant for NewExpression and probably many other.
I propose to remove the blank all the time if there is no alpha character at openParen.pos - 1. That includes async (a) => b and export default (a) => b.
Another rule will probably complain about a missing blank, but that's not the business of this rule.

@mateuszwitkowski
Copy link
Contributor Author

@ajafff I've updated my PR. Apart from checking for alpha characters fixer checks also assignemnt character. Otherwise it would fix var a = (a) => {} to var a =a => {}

@@ -72,8 +72,10 @@ function walk(ctx: Lint.WalkContext<Options>) {
}
} else if (ctx.options.banSingleArgParens) {
const closeParen = getChildOfKind(node, ts.SyntaxKind.CloseParenToken)!;
const charBeforeOpenParen = ctx.sourceFile.text.substring(openParen.pos - 1, openParen.pos);
const replaceValue = charBeforeOpenParen.match(/[a-z=]/i) !== null ? " " : "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this rule should do anything related to code formatting. It adds the space only where strictly necessary to avoid invalid syntax.
Everything else assumes a certain style preference of the user, that this rule cannot know.

Anyways, if you really want to add a space after = you should probably also add one after

  • , as in foo(bar, (a) => b)
  • ? and : as in foo ? (a) => b : (a) => c
  • & and | as in someFn || (a) => b
  • maybe more

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right about unnecessary code formatting here. I'll change it and adjust test cases accordingly.

arr.map((a) => b);
~ [0]

foo(async (a) => b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add some more tests like

function *gen() { yield (a) => b; }
export default (a) => b;
throw (a) => b; // hopefully nobody ever does this

@ajafff ajafff merged commit cfa76a2 into palantir:master Jan 2, 2018
@ajafff
Copy link
Contributor

ajafff commented Jan 2, 2018

Thanks @mateuszwitkowski

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants