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

sort the edits in descending order #289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ananthakumaran
Copy link
Owner

@ananthakumaran ananthakumaran commented Nov 23, 2018

fixes #288

Applying the edit instructions in reverse to file will result in correctly reformatted text.

One of the comments in TypeScript repo says the edits should be applied
in reverse order. But, I am not sure all the plugins follow this? Doing
a manual sort does seem to fix the tslint issue. VS code applies the fixes
correctly, but I can't able to find the logic. Atom[1] also manually
sorts the edits.

1: https://github.com/TypeStrong/atom-typescript/blob/master/lib/main/pluginManager.ts#L349-L352

fixes #288

> Applying the edit instructions in reverse to file will result in correctly reformatted text.

One of the comments in TypeScript repo says the edits should be applied
in reverse order. But, I am not sure all the plugins follows this? Doing
a manual sort does seem to fix the tslint issue. VS code applies the fixes
correctly, but I can't able to find the logic. Atom[1] also manually
sorts the edits.

1: https://github.com/TypeStrong/atom-typescript/blob/master/lib/main/pluginManager.ts#L349-L352
@ananthakumaran
Copy link
Owner Author

@jupl could you try this branch?

@jupl
Copy link

jupl commented Nov 24, 2018

I can still replicate the issue unfortunately. :(

@ananthakumaran
Copy link
Owner Author

ananthakumaran commented Nov 25, 2018 via email

@jupl
Copy link

jupl commented Nov 25, 2018

https://github.com/jupl/tide-thingy

I can fix the var keyword with tide-fix, but attempting to fix curly spacing with s or d gets wacky. It works as expected in VSCode.

* don't format after the application of code-edit
* group the code-edits by filename, so the changes for the same
  file will be processed together.
@ananthakumaran
Copy link
Owner Author

ananthakumaran commented Nov 25, 2018

Thanks for the example, it was helpful in debugging. I have added two more fixes

  1. fixes for same file are sent as two different code-edits by the plugin, I have to do a group by filename to fix this
  2. we apply format-region on the region where the new code-edits are applied. This was a hack to prevent code-edit messing up the format, but in your case, tide-format was adding the spaces back. I have removed the tide-format.

I am kind of not sure about merging these changes. AFAIK, these things are not thoroughly documented anywhere, so I am of not sure what is the correct fix and who is at fault here.

@ananthakumaran
Copy link
Owner Author

ananthakumaran commented Nov 25, 2018

    /** Apply each textChange in order, updating future changes to account for the text offset of previous changes. */
    function forEachTextChange(changes: ReadonlyArray<ts.TextChange>, cb: (change: ts.TextChange) => void): void {
        // Copy this so we don't ruin someone else's copy
        changes = JSON.parse(JSON.stringify(changes));
        for (let i = 0; i < changes.length; i++) {
            const change = changes[i];
            cb(change);
            const changeDelta = change.newText.length - change.span.length;
            for (let j = i + 1; j < changes.length; j++) {
                if (changes[j].span.start >= change.span.start) {
                    changes[j].span.start += changeDelta;
                }
            }
        }
    }

from this logic it looks like applying the changes in reverse order should work, as there won't be any more span that will be affected by the current one

@ananthakumaran
Copy link
Owner Author

@jupl could you test it once more

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

Successfully merging this pull request may close these issues.

tide-fix and typescript-tslint-plugin
2 participants