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

Swap diff for faster-diff #4816

Closed
wants to merge 7 commits into from
Closed

Swap diff for faster-diff #4816

wants to merge 7 commits into from

Conversation

tmcw
Copy link

@tmcw tmcw commented Jul 6, 2018

Refs #4801

The faster-diff module appears to be seriously modeled after the O(ND) difference paper, whereas I'm not sure about the diff implementation. This switches prettier to faster-diff. faster-diff is much faster - around 20x speedup.

But:

  • This uses a null byte (\0) as the cursor position. I'm not sure how else to represent the cursor in strings. I wonder, given the strategy undertaken here, whether it wouldn't just be simpler to split the old string at the cursor position?
  • It fails on current tests. faster-diff behavior doesn't match diff behavior, in subtle ways.

@j-f1 j-f1 added the status:wip Pull requests that are a work in progress and should not be merged label Jul 6, 2018
@j-f1 j-f1 changed the title [wip] Swap diff for faster-diff Swap diff for faster-diff Jul 6, 2018
@tmcw
Copy link
Author

tmcw commented Jul 9, 2018

I'm not seeing a clear route to making faster-diff's behavior exactly match diff's behavior without gutting it - how exactly does this cursor offsetting need to be? Also, any ideas about whether there's some UTF8 character that would be useful as a cursor standin?

@j-f1
Copy link
Member

j-f1 commented Jul 9, 2018

how exactly does this cursor offsetting need to be?

As exact as possible :) Can you run yarn test -u so we can more easily see what the changes are and see how we feel?

Also, any ideas about whether there's some UTF8 character that would be useful as a cursor standin?

Perhaps one of the non-characters?

@j-f1
Copy link
Member

j-f1 commented Jul 9, 2018

After looking through the test failures, it looks like cursor position can differ by parser (according to the current behavior in master). Since this PR improves consistency, I’m feeling better about merging it.

@tmcw
Copy link
Author

tmcw commented Aug 8, 2018

This fell off the radar for a moment, but I figured out how to update the tests properly - and rebased on master.

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Changes to the output appear reasonable.

@tmcw
Copy link
Author

tmcw commented Oct 10, 2018

Anything else to fix up so that this is mergable?

@j-f1
Copy link
Member

j-f1 commented Oct 12, 2018

Looks like you’ve got conflicts :/

Copy link
Contributor

@vjeux vjeux left a comment

Choose a reason for hiding this comment

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

We've had very little usage of the cursor preservation code so it's not surprising that it's very slow. The changes are unfortunate but I think it's not the end of the world since they only seem to appear when optional punctuation is being injected, so it's not the end of the world (would be nice if it got fixed though).

Thanks for doing this!

@@ -208,7 +207,7 @@ thisWontBeFormatted ( 1 ,3)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
thisWontBeFormatted ( 1 ,3)

thisWillBeFormatted(2, 3<|>);
thisWillBeFormatted(2, <|>3);
Copy link
Contributor

Choose a reason for hiding this comment

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

None of those changes seem right.

thisWillBeFormatted    (2  ,3,  <|> )

If your cursor was after the 3, you don't want it to jump before the 3.

const y = 5;
<|>
const y = 5<|>;

Copy link
Contributor

Choose a reason for hiding this comment

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

The cursor was on the line after the 5, you don't want it to become in the same line as the 5

@tmcw
Copy link
Author

tmcw commented Mar 8, 2019

Is the idea that most editor extensions do their own cursor logic instead of using Prettier's functionality? It looks like the VSCode extension does its own thing, whereas the Atom one uses formatWithCursor.

@duailibe
Copy link
Member

Is the idea that most editor extensions do their own cursor logic instead of using Prettier's functionality?

I have no idea. I know I use formatWithCursor in my integration

@tmcw
Copy link
Author

tmcw commented Sep 3, 2019

Closing in favor of #6164, though it's unclear whether that one'll ever land.

@tmcw tmcw closed this Sep 3, 2019
@lipis lipis added this to the 1.19 milestone Sep 11, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Dec 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:wip Pull requests that are a work in progress and should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants