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

Reduce how much text needs to be diffed when computing cursor positions #15709

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

ExplodingCabbage
Copy link
Contributor

@ExplodingCabbage ExplodingCabbage commented Nov 23, 2023

Description

This implements roughly the tactic I proposed at #4801 (comment).

It's a draft for now. I have a bunch of outstanding concerns I want to dig into before it's ready for review (in addition to the stuff in the PR template checklist), many of which probably require tweaks to the logic here:

  • I've changed the signatures of a bunch of functions. Are any of them part of the package's API, meant to be used either by end users of Prettier or authors of third-party plugins? I don't have a clear idea yet of what's an internal implementation detail vs part of the interface for plugin authors vs part of the interface for end users. - Edit: I've undone the removal of opts.cursorNode to address this.
    • In particular: I removed opts.cursorNode, but perhaps that's something that third party plugins might use or modify, in which case I've broken them?
  • I seem to have broken a couple of the tests that combine cursor positioning with formatting only a target range. I am a bit confused about why those tests exist because the Pretter CLI --help text specifically says that those two features can't be used together. (I also have no idea why my changes broke that scenario in particular.) I guess I should investigate...
  • I've added some logic to printAstToDoc to insert the cursor symbol at beginning or end of the doc in the scenario where the cursor is located before the first AST node. But what happens when there are embedded docs, or when printAstToDoc gets called from the multiparser (are these the same scenario?)? Since I didn't consider these scenarios at all when writing the code, I presume the behaviour of my code in them is currently broken.
  • Have I introduced a possible scenario where only one cursor placeholder gets printed in printDocToString (e.g. because the other is part of a group that doesn't get printed at all for some reason)? If this happens it will cause printDocToString to throw, so I need to either ensure it's impossible or modify printDocToString to handle it when it happens.
  • What happens when the nodeBeforeCursor ends up after nodeAfterCursor in the formatted doc, e.g. because some sort of sorting plugin for Prettier was used like https://github.com/trivago/prettier-plugin-sort-imports? This obviously breaks the intended logic for computing the cursor position, and right now I don't detect it or recover from it.
  • Wherever it makes sense, I should add tests exploring the precise doubts I list above (Edit: done in Add a bunch more cursor position test cases #15787)

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

commands.md Outdated Show resolved Hide resolved
@ExplodingCabbage
Copy link
Contributor Author

@fisker just in case you're not aware, note this is ready for review and has been for a while.

@fisker
Copy link
Member

fisker commented Jan 18, 2024

What happens when the nodeBeforeCursor ends up after nodeAfterCursor in the formatted doc

Is this the reason why you add cursorStart/cursorEnd?

@ExplodingCabbage
Copy link
Contributor Author

Ooh, sorry, missed the comment above somehow.

Yeah, I think the new cursorStart & cursorEnd only need to exist to handle the scenario where a third-party plugin reorders AST nodes. In every other situation, it'd be fine if we just had cursor.

@fisker
Copy link
Member

fisker commented Feb 16, 2024

I'm not sure it worth to make the change (cursor -> cursor{Start,End}).

a) plugins maybe already using cursor
b) builtin plugin doesn't reorder nodes, and we don't encourage plugin to do that either

@ExplodingCabbage
Copy link
Contributor Author

Fair enough; I've removed the new cursorStart and cursorEnd in favour of just using cursor.

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.

None yet

2 participants