Add `cursorOffset` option for cursor translation #1637

josephfrazier · 2017-05-20T03:46:34Z

This addresses #93 by
adding a new option, cursorOffset, that tells prettier to determine
the location of the cursor after the code has been formatted. This is
accessible through the API via a new function, formatWithCursor, which
returns a {formatted: string, cursorOffset: ?number}.

Here's a usage example:

require('prettier').formatWithCursor(' 1', {cursorOffset: 1})
// -> { formatted: '1;\n', cursorOffset: 0 }

This implementation works roughly as described in #93 (comment).
It doesn't yet have tests, but I wanted to go ahead and get some
feedback on the API to see if this seems like the right way to go.

Thoughts?

vjeux · 2017-05-20T05:25:56Z

Omg this is so exciting! I'll review this when I get a chance

CiGit · 2017-05-20T08:02:35Z

Shouldn't a cursorOffset be a tuple [line, column]?
Would line/column be 0 or 1 based ?

josephfrazier · 2017-05-20T13:42:59Z

Thanks @vjeux, I'm looking forward to the feedback!

@CiGit, since the input text is a single string, rather than an array of lines, I decided to use a single zero-based integer as the cursorOffset. A [line, column] tuple could also work, but it would have to be converted back into a single integer anyway. Besides, I'm guessing editors integrating with this feature already keep track of the single-integer offset, or can easily generate it given the [line, column].

vjeux · 2017-05-21T16:16:20Z

index.js

@@ -1,5 +1,8 @@
 "use strict";

+const traverse = require('babel-traverse').default;


does it work for flow AST?

Ah, it didn't, but I added support in cbc1929

vjeux · 2017-05-21T16:30:54Z

Sorry I don't have the time to review it right now, I'll take another pass later.

See prettier#1637 (comment) For example: prettier tests/flow/any/any.js --cursor-offset 40 --parser flow # prints 44

josephfrazier · 2017-05-21T19:50:19Z

No worries, I still have to refactor this to make it work with the range formatting changes (#1609) anyway.

vjeux · 2017-05-24T20:20:59Z

Should I wait for you to refactor it based on the findings in the other diff? I'd love to get this feature in!

josephfrazier · 2017-05-24T20:45:32Z

I haven't done any further work on this yet, so if you'd like to, feel free! The range-formatting feature does make it more complex, though, since this change introduces a separate formatWithCursor function and moves the shebang handling into it.

Also note that if a range is specified the cursor could technically be inside or outside of it, rather than just on one of its endpoints, but that's not likely to happen in practice, so we can probably ignore those cases for now.

EDIT: Now that I think about it, range-formatting probably doesn't currently take shebang handling into account, meaning that the formatted range will be offset by the shebang length...

That way, we won't have to do any arithmetic on range and cursor locations when there is a shebang. See here for details: prettier#1637 (comment) This required changing comment printing such that comments that are actually shebangs are just ignored.

* Verify shebang tests against Babylon as well as Flow/Typescript * Pass entire text through formatWithShebang() to format() That way, we won't have to do any arithmetic on range and cursor locations when there is a shebang. See here for details: #1637 (comment) This required changing comment printing such that comments that are actually shebangs are just ignored. * Add curly brace after `if` See #1718 (comment)

This addresses prettier#93 by adding a new option, `cursorOffset`, that tells prettier to determine the location of the cursor after the code has been formatted. This is accessible through the API via a new function, `formatWithCursor`, which returns a `{formatted: string, cursorOffset: ?number}`. Here's a usage example: ```js require("prettier").formatWithCursor(" 1", { cursorOffset: 2 }); // -> { formatted: '1;\n', cursorOffset: 1 } ```

It will print out the offset instead of the formatted output. This makes it easier to test. For example: echo ' 1' | prettier --stdin --cursor-offset 2 # prints 1

josephfrazier · 2017-05-30T16:10:26Z

index.js

  addAlignmentSize = addAlignmentSize || 0;

  const ast = parser.parse(text, opts);

  const formattedRangeOnly = formatRange(text, opts, ast);
  if (formattedRangeOnly) {
-    return formattedRangeOnly;
+    return { formatted: formattedRangeOnly };
+  }


The cursor-translation functionality still needs to be unified with the range-formatting functionality (currently, either one or the other can be used). I think it might be easiest to do this by keeping track of the range start/end positions in the same way as the cursor position, then having an API that returns all 3 positions (if provided), and only formats the text necessary to satisfy the range.

josephfrazier · 2017-06-01T21:25:48Z

@vjeux, is there any chance this can be merged and land in tomorrow's release? It definitely still needs some work (as mentioned in #1637 (comment)), but releasing it would allow editor integrations to start building upon it.

vjeux · 2017-06-01T21:36:25Z

src/printer.js

      path,
      p => genericPrint(p, options, printGenerically, args),
      options,
      args && args.needsSemi
    );
+
+    if (path.getNode() === options.cursorNode) {


You probably want to put this inside of printComments, there are a few places where we don't print the node but do print the comments.

Ah, that hadn't occurred to me. Fixed in d75906d

vjeux · 2017-06-01T21:38:11Z

bin/prettier.js

    }
  });
 }

+function writeOutput(result) {
+  if (options.cursorOffset) {
+    console.log(result.cursorOffset);


Do we just output the cursor offset from CLI? We don't output the formatted text? Could we print the formatted text on stdout and the cursor on stderr?

Oh, good idea, I hadn't thought of that. Fixed in 8f72d51

vjeux · 2017-06-01T21:39:04Z

Okay let's ship it tomorrow. I just asked a few questions.

vjeux · 2017-06-01T21:40:22Z

bin/prettier.js

@@ -202,7 +204,7 @@ function format(input, opt) {
    return;
  }

-  return prettier.format(input, opt);
+  return prettier.formatWithCursor(input, opt);


I'm really upset that we have to introduce a new api and didn't think about this when we built .format :(

:/ It's a somewhat unwieldy name, too

vjeux · 2017-06-01T21:41:55Z

src/options.js

@@ -4,6 +4,7 @@ const validate = require("jest-validate").validate;
 const deprecatedConfig = require("./deprecated");

 const defaults = {
+  cursorOffset: -1,


shouldn't it be undefined here?

Yeah, undefined would make more sense. However, when I tried it, I got a validation error:

FAIL tests/cursor/jsfmt.spec.js ● translates cursor correctly in basic case Error ● Validation Error: Option "cursorOffset" must be of type: undefined but instead received: number Example: { "cursorOffset": undefined } ✕ translates cursor correctly in basic case (3ms)

Here's the diff I tested:

diff --git a/index.js b/index.js index 5a66c9cb..e9e67231 100644 --- a/index.js +++ b/index.js @@ -64,7 +64,7 @@ function formatWithCursor(text, opts, addAlignmentSize) { } let cursorOffset; - if (opts.cursorOffset >= 0) { + if (opts.cursorOffset !== undefined) { const cursorNodeAndParents = findNodeAtOffset(ast, opts.cursorOffset); const cursorNode = cursorNodeAndParents.node; if (cursorNode) { diff --git a/src/options.js b/src/options.js index 73d40426..956168af 100644 --- a/src/options.js +++ b/src/options.js @@ -4,7 +4,7 @@ const validate = require("jest-validate").validate; const deprecatedConfig = require("./deprecated"); const defaults = { - cursorOffset: -1, + cursorOffset: undefined, rangeStart: 0, rangeEnd: Infinity, useTabs: false,

Is there a good way to get around this?

Ohhh, okay. Can you not put it in defaults? Or somehow make it optional there?

Otherwise don't worry, it's fine having -1

I could remove it from defaults, but then there's a validation warning:

console.warn node_modules/jest-validate/build/utils.js:43 ● Validation Warning: Unknown option "cursorOffset" with value 2 was found. This is probably a typing mistake. Fixing it will remove this message.

Not sure if there's a way to make it optional, so I'll leave it as-is for now. We can always revisit it later.

vjeux · 2017-06-01T21:42:16Z

index.js

+  }
+
+  let cursorOffset;
+  if (opts.cursorOffset >= 0) {


!== undefined

(for future reference, the discussion about this is above)

vjeux · 2017-06-01T21:42:52Z

index.js

+    const cursorNode = cursorNodeAndParents.node;
+    if (cursorNode) {
+      cursorOffset = opts.cursorOffset - util.locStart(cursorNode);
+      opts.cursorNode = cursorNode;


don't mutate please

Hmm, we're already mutating elsewhere with opts.newLine = guessLineEnding(text); and opts.originalText = text.trimRight();. I can take a look refactoring it tomorrow though, if you're set on this.

So sad. Would be nice to refactor but I can live with this for this PR.

vjeux · 2017-06-01T21:43:25Z

src/doc-printer.js

@@ -411,6 +416,16 @@ function printDocToString(doc, options) {
      }
    }
  }
+
+  const cursorPlaceholderIndex = out.indexOf(cursor.placeholder);
+  if (cursorPlaceholderIndex >= 0) {


fixed in c4e08d6

vjeux · 2017-06-01T21:47:06Z

src/doc-printer.js

+  const cursorPlaceholderIndex = out.indexOf(cursor.placeholder);
+  if (cursorPlaceholderIndex >= 0) {
+    const beforeCursor = out.slice(0, cursorPlaceholderIndex).join("");
+    options.cursorOffsetResult = beforeCursor.length;


Don't mutate. Change the API such that it returns {formatted, cursor}

That's much cleaner :) Fixed in 1fa6b4e

This lets us continue to print the formatted code, while also communicating the updated cursor position. See prettier#1637 (comment)

See prettier#1637 (comment)

…g options See prettier#1637 (comment)

vjeux · 2017-06-01T22:42:21Z

Is it ready to review again?

josephfrazier · 2017-06-01T22:47:22Z

Yup, I'll follow up on any feedback tomorrow.

vjeux · 2017-06-01T22:52:39Z

Thanks for this! It's going to be awesome <3

josephfrazier · 2017-06-02T15:54:20Z

Thanks for merging on such short notice before the release! I'll create a new issue for the task in #1637 (comment).

Since the `cursorOffset` option (introduced in prettier#1637) works by tracking the cursor position relative to an AST node (rather than a CST token), it can produce incorrect results. See prettier#1981

* Add test demonstrating SourceElement-relative cursor translation See #1981 (comment) * Translate cursor relative to nearest node, not SourceElement This partially address #1981 See #1981 (comment) * Add test demonstrating incorrect cursor translation Since the `cursorOffset` option (introduced in #1637) works by tracking the cursor position relative to an AST node (rather than a CST token), it can produce incorrect results. See #1981

josephfrazier mentioned this pull request May 20, 2017

Translate cursor position #93

Closed

vjeux reviewed May 21, 2017

View reviewed changes

josephfrazier added a commit to josephfrazier/prettier that referenced this pull request May 21, 2017

Support flow/typescript parsers with --cursor-offset

cbc1929

See prettier#1637 (comment) For example: prettier tests/flow/any/any.js --cursor-offset 40 --parser flow # prints 44

This was referenced May 21, 2017

Add --range-start and --range-end options to format only parts of the input #1609

Merged

Find nearest node when formatting range #1659

Merged

josephfrazier mentioned this pull request May 24, 2017

Pass entire text through formatWithShebang() to format() #1718

Merged

vjeux changed the title ~~(WIP) Add cursorOffset option for cursor translation~~ [WIP] Add cursorOffset option for cursor translation May 25, 2017

josephfrazier added 2 commits May 30, 2017 12:02

Add --cursor-offset CLI option

3ea9d1b

It will print out the offset instead of the formatted output. This makes it easier to test. For example: echo ' 1' | prettier --stdin --cursor-offset 2 # prints 1

josephfrazier force-pushed the cursor branch from cbc1929 to 3ea9d1b Compare May 30, 2017 16:05

josephfrazier commented May 30, 2017

View reviewed changes

josephfrazier added 3 commits June 1, 2017 17:00

Add basic test of cursor translation

1c5ce2f

Merge branch 'master' into cursor

6b8db51

Document cursorOffset option and formatWithCursor()

7717d97

vjeux reviewed Jun 1, 2017

View reviewed changes

josephfrazier added 4 commits June 1, 2017 17:55

Merge branch 'master' into cursor

9502fcf

Print translated cursor offset to stderr when --cursor-offset is given

8f72d51

This lets us continue to print the formatted code, while also communicating the updated cursor position. See prettier#1637 (comment)

doc-print cursor placeholder in comments.printComments()

d75906d

See prettier#1637 (comment)

Compare array index to -1 instead of >= 0 to determine element presence

c4e08d6

See prettier#1637 (comment)

josephfrazier changed the title ~~[WIP] Add cursorOffset option for cursor translation~~ Add cursorOffset option for cursor translation Jun 1, 2017

Return {formatted, cursor} from printDocToString() instead of mutatin…

1fa6b4e

…g options See prettier#1637 (comment)

vjeux merged commit 4bfeb90 into prettier:master Jun 1, 2017

josephfrazier deleted the cursor branch June 2, 2017 07:39

This was referenced Jun 2, 2017

Allow cursorOffset to be used with rangeStart and rangeEnd #1869

Closed

Prettify as you type prettier/prettier-vscode#12

Closed

CiGit mentioned this pull request Jun 2, 2017

Preserve indentation when formatting selection prettier/prettier-vscode#113

Merged

josephfrazier mentioned this pull request Jun 5, 2017

--cursor-offset output is wrong sometimes #1981

Closed

renovate bot mentioned this pull request Jun 7, 2017

Update dependency prettier to version 1.4.4 renovatebot/renovate#284

Merged

josephfrazier mentioned this pull request Jun 8, 2017

Factor a prettier printer into separate library #2029

Closed

bakkot mentioned this pull request Jul 4, 2017

--debug-print-doc is broken #2396

Closed

ryanhiebert mentioned this pull request Jul 8, 2017

Loses cursor position on format prettier/prettier-atom#34

Closed

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 20, 2019

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

		@@ -1,5 +1,8 @@
		"use strict";

		const traverse = require('babel-traverse').default;

Uh oh!

Add cursorOffset option for cursor translation #1637

Add cursorOffset option for cursor translation #1637

Uh oh!

Conversation

josephfrazier commented May 20, 2017

Uh oh!

vjeux commented May 20, 2017

Uh oh!

CiGit commented May 20, 2017

Uh oh!

josephfrazier commented May 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented May 21, 2017

Uh oh!

josephfrazier commented May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux commented May 24, 2017

Uh oh!

josephfrazier commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephfrazier May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephfrazier commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Jun 1, 2017

Uh oh!

josephfrazier commented Jun 1, 2017

Uh oh!

vjeux commented Jun 1, 2017

Add `cursorOffset` option for cursor translation #1637

Add `cursorOffset` option for cursor translation #1637

josephfrazier commented May 21, 2017 •

edited

Loading

josephfrazier commented May 24, 2017 •

edited

Loading

josephfrazier May 30, 2017 •

edited

Loading