Skip to content
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

Refactor removes whitespace #27294

Closed
markusjohnsson opened this issue Sep 23, 2018 · 11 comments · Fixed by #36688
Closed

Refactor removes whitespace #27294

markusjohnsson opened this issue Sep 23, 2018 · 11 comments · Fixed by #36688
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone

Comments

@markusjohnsson
Copy link
Contributor

Refactors, like 'move to a new file', butchers formatting by removing empty lines in code. As refactors are meant to improve code style and structure, I should not manually have to reformat my code after I've applied one.

TypeScript Version: 3.1.0-dev.20180922

Search Terms:
Move to new file removes newlines, refactor removes newlines, removes whitespace, removes trivia

Code

function example() {
    thisIsAnExampleWithSomeSpace();

    anotherCallToSomeFunctionHere();

    evenMoreStatementsLater();
}

Select function and 'move to a new file' in VS Code:
Expected behavior:
Same formatting:

export function example() {
    thisIsAnExampleWithSomeSpace();

    anotherCallToSomeFunctionHere();

    evenMoreStatementsLater();
}

Actual behavior:
Blank lines removed:

export function example() {
    thisIsAnExampleWithSomeSpace();
    anotherCallToSomeFunctionHere();
    evenMoreStatementsLater();
}

Related Issues:
Perhaps related to #843

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Sep 24, 2018
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 24, 2018
@RyanCavanaugh RyanCavanaugh assigned ghost Oct 10, 2018
@ghost ghost modified the milestones: TypeScript 3.2.1, Future Nov 16, 2018
@ghost ghost removed their assignment Nov 16, 2018
@mjbvz mjbvz added the VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone label May 21, 2019
@markusjohnsson
Copy link
Contributor Author

This really is super annoying, so much that the much awaited "Move to new file" is practically useless - I prefer cutting & pasting manually to preserve formatting..

How large of a job is this to fix? Is it something that is doable for a one-time-contributor or would one need a deep understanding of parser, AST and emitter?

@cinderblock
Copy link

cinderblock commented Jan 23, 2020

An adequate shortcut I've found is to add two simple steps:

  1. Select function you want to move, Copy
  2. Use "Move to a new file" feature. (Brings you to the new file as well)
  3. Select new function, Paste

This leaves the minor problem of whitespace preservation around imports.

@DanielRosenwasser
Copy link
Member

@RyanCavanaugh spoke offline with @mjbvz and I - while this doesn't seem like the desired behavior, this is going to involve a larger amount of work than it might seem since we're not equipped in our current refactoring machinery.

@markusjohnsson
Copy link
Contributor Author

@DanielRosenwasser does that mean that it's not going to happen?

It really is too bad, since "the editor understanding and working with the code" is one of the selling points for typescript over javascript, at least for me. Great refactoring support is where typescript could really shine.

@DanielRosenwasser
Copy link
Member

No, I didn't mean to imply it wasn't going to happen, just that it's not happening in 3.8.

@markusjohnsson
Copy link
Contributor Author

markusjohnsson commented Mar 6, 2020

I want to note that refactors not only removes blank lines, but alters other whitespace as well:

Before:

function Foo({ label }: { label: string }) {
    return (
        <div
            id="time-label-top-container">
            <div
                id="time-label-container"
                style={{
                    marginRight: '10px',
                    border: 'none',
                }}
            >
                <div className="currentTimeLabel">{label}</div>
            </div>
        </div>
    );
}

After (move to a new file):

import React from "react";
function Foo({ label }: {
    label: string;
}) {
    return (<div id="time-label-top-container">
        <div id="time-label-container" style={{
            marginRight: '10px',
            border: 'none',
        }}>
            <div className="currentTimeLabel">{label}</div>
        </div>
    </div>);
}

If I'm reading the proposed fix correctly, it will only provide newlines. Other formatting will still get lost.

@andrewbranch
Copy link
Member

That is a correct-ish reading of the fix, but the example you just provided is really all about newlines. That said, this example works horribly in my branch, exposing some bugs in the implementation, so I’ll incorporate this as a test case.

@markusjohnsson
Copy link
Contributor Author

@andrewbranch Yeah, kind of realized after I posted, but there is also indentation and the added newlines in the function parameters. Glad I could provide a test case. Is it possible for me to try your branch with VS Code refactorings?

@minestarks
Copy link
Member

FYI @jessetrinity working on a related class of bugs

@AbdelrahmanHafez
Copy link

AbdelrahmanHafez commented Apr 26, 2020

if (true) console.log('On the same line');

becomes

if (true)
  console.log('On the same line');

Does this PR handle this issue? I used TS 3.9 as @mjbvz recommended in this comment, the empty lines issue is solved, but the moving to the next line isn't.

Should I create a new issue with this?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 26, 2020

Please try out https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-typescript-next and file a separate issue with repro steps if that doesn't fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants