Skip to content

Indentation is busted for extract function #18091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DanielRosenwasser opened this issue Aug 28, 2017 · 6 comments
Closed

Indentation is busted for extract function #18091

DanielRosenwasser opened this issue Aug 28, 2017 · 6 comments
Labels
Bug A bug in TypeScript Domain: Formatter The issue relates to the built-in formatter Domain: Refactorings e.g. extract to constant or function, rename symbol Fixed A PR has been merged for this issue

Comments

@DanielRosenwasser
Copy link
Member

export function interpret(expr: ast.Expression): number {
    switch (expr.kind) {
        case "BinaryExpression":
            const { left, right } = expr;
            return interpret(left) + interpret(right);
    }
}

Extract the entire function body out into its own function.

Expected:

export function interpret(expr): number {
    return newFunction(expr);
}

function newFunction(expr: any) {
    switch (expr.kind) {
        case "BinaryExpression":
            const { left, right } = expr;
            return interpret(left) + interpret(right);
    }
}

Actual:

export function interpret(expr): number {
    return newFunction();

    function newFunction() {
        switch(expr.kind) {
        case "BinaryExpression":
        const { left, right } = expr;
        return interpret(left) + interpret(right);
    }
    }
}
@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: Formatter The issue relates to the built-in formatter Domain: Refactorings e.g. extract to constant or function, rename symbol labels Aug 28, 2017
@mhegazy mhegazy assigned ghost Aug 28, 2017
@mhegazy mhegazy added this to the TypeScript 2.5.3 milestone Aug 28, 2017
@DanielRosenwasser
Copy link
Member Author

Another broken case:

export function evaluate(node: any): number {
    switch (node.kind) {
        case "Parenthesized":
            while (node.expression.kind === "Parenthesized") {
                node = node.expression;
            }
            return evaluate(node.expression);
    }
}

Gives me:

export function evaluate(node: any): number {
    switch (node.kind) {
        case "Parenthesized":
            var __return: any;
            ({ __return, node } = newFunction(node));
            return __return;
    }
}
function newFunction(node: any) {
while(node.expression.kind === "Parenthesized") {
    node = node.expression;
}
    return { __return: evaluate(node.expression), node };
}

@amcasey
Copy link
Member

amcasey commented Aug 30, 2017

It's worth mentioning that the repro is from VS Code - it works in VS.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 30, 2017

does it work in VS because we always call format after applying the change?

@amcasey
Copy link
Member

amcasey commented Aug 30, 2017

VS does format after applying the change. I don't know whether that's why it works in VS.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 30, 2017

yeah. that is why. in vs code if you trigger a format later manually it will be fixed. looks like our base indentation gets messed up when we format the new tree.

@drake7707
Copy link

I've noticed the same thing happens with the suggested text changes of code fixes, such as implementing the abstract methods from an abstract class

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: Formatter The issue relates to the built-in formatter Domain: Refactorings e.g. extract to constant or function, rename symbol Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants