Fix empty line check between array elements #14736

solarized-fox · 2023-04-21T11:25:02Z

Description

it seems that parens are not included in the start-end range a node has for referencing the source text. this can cause skipToLineEnd to compute incorrectly because it isn't expecting to encounter any trailing ")"s. adding ")" to the list of characters it skips fixes this

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✨

fisker · 2023-04-21T12:28:26Z

Honestly, I didn't expect you to change this part ...

It's also a public API.
If we accept this change, we'll need this changelog to show that skipToLineEnd and isNextLineEmpty skips ).

But before that, we need to think about what will be affected.

fisker · 2023-04-21T12:30:51Z

This case is not format correctly

Prettier pr-14736
Playground link

--parser babel

Input:

[
  (a=b
  
  ),
  b, //
];

Output:

[
  (a = b),

  b, //
];

solarized-fox · 2023-04-21T14:06:29Z

Honestly, I didn't expect you to change this part ...

I tried a few other approaches, but they caused tests to fail. if you have any ideas I'm open to suggestions!

But before that, we need to think about what will be affected.

as for this, my understanding of skipToLineEnd is that it supposes that the only non-comment characters between the last node and EOL are in the character class of "\t ,; ", which would mean that since ")" can appear in there, this is not actually a behavior change so much as a bug fix. that's why I didn't make a changelog, but I can add one if you believe it's important

as for:

This case is not format correctly

Input:

[
  (a=b
  
  ),
  b, //
];

Output:

[
  (a = b),

  b, //
];

I'm not sure the information required to fix this is present in the AST? the existing code uses the next line being whitespace as a sort of hacky heuristic here, but as you've demonstrated that is inadequate. any robust solution would require a fair bit of code to determine what is and isn't part of a multiline expression. perhaps this is better off raised as a followup issue, as it doesn't seem idempotency is violated in this example and fixing this beyond all edge cases might be a nontrivial effort requiring discussion?

solarized-fox · 2023-04-22T18:05:40Z

I decided to force-push a different version of this change. it solves the edge case mentioned by @fisker and doesn't change any public APIs. if anyone knows a shorter way to do this I'm all ears but otherwise I think this is an improvement over the last solution

solarized-fox · 2023-04-23T07:29:22Z

I noticed there was a trivial merge conflict (printArrayItems was renamed to printArrayElements while I was working on it) so I fixed that up and force-pushed the change. it should be automatically mergeable now

fisker · 2023-04-23T10:59:35Z

src/language-js/print/array.js

+      } else {
+        const nextCandidate =
+          text[currentIdx] == ")" ? currentIdx + 1 : candidateIdx;
+        return skipParens(skipIfComment(currentIdx + 1), nextCandidate);


Can skip function do the job?

I think it can't replace skipParens (and I think I'll rename skipParens to indexAfterFurthestParen for clarity) however after looking into it skipIfComment can be implemented with skip calls which cuts down a fair bit of code, so I'll make that change

src/language-js/print/array.js

solarized-fox · 2023-04-23T14:28:58Z

while working editing the code, I realized instead of skipping the parens I can just skip directly to the comma. this is a lot less code overall

however, it did trigger one change in the tests where

[
  1
  ,
  
  2,
]

used to format as [1, 2] and now formats as [1,\n2]. this seems like it should be the correct behavior though, because otherwise a newline gets eaten. I have added a changelog and updated tests accordingly

solarized-fox · 2023-04-24T02:33:28Z

thinking about the change, I guess the root question is, for this input:

_ = [
  (a = b
  //comment
  ),

  2,
  3 //
]

which of these outputs is most appropriate?

git checkout #13017

_ = [
  (a = b),

  //comment
  2,
  3, //
];

git checkout #13017-2

_ = [
  (a = b),
  //comment
  2,
  3, //
];

looking at it now, it probably would be the second example. the comment gets separated otherwise. I can push 13017-2 onto this branch if you think that'd be an improvement

fisker · 2023-04-24T02:46:52Z

Expected output should be

_ = [
  (a = b), //comment

  2,
  3, //
];

It's bug in comment attach. Should be done in a separate PR. The blank line check is correct here.

solarized-fox · 2023-04-24T03:00:05Z

I see. well, setting aside the comment then, for this input:

_ = [
  (a = b
  ),

  2,
  3 //
]

which of these is preferred?

git checkout #13017

_ = [
  (a = b),

  2,
  3, //
];

git checkout 13017-2

_ = [
  (a = b),
  2,
  3, //
];

fisker · 2023-04-24T03:04:00Z

_ = [
  (a = b),

  2,
  3, //
];

Because there is a blank line between comma after first element and second element.

solarized-fox · 2023-04-24T03:12:38Z

alright then, I'm fairly confident that this (the branch this PR is attached to) is the correct change. solving the root issue (i.e. #13017) without changing any snapshots would require writing code that then needs to be thrown out later. if you find anything else in the PR that needs changing, let me know, but otherwise I think it's ready

…etween the element and the comma

solarized-fox · 2023-04-24T04:21:03Z

oops, saw the lint failure, fixed it and ran lint locally too so it should be solved for good

src/language-js/print/array.js

fisker

LGTM, thanks!

solarized-fox · 2023-04-25T18:43:09Z

ah, sorry for the sudden force push, I accidentally pushed the button to merge changes from main into this branch, but I rolled that back. is there something else I need to do or will these changes be merged at some point in the future? I'm not a maintainer on this project, so I cannot merge it myself

sosukesuzuki

LGTM! Please check one comment.

sosukesuzuki · 2023-05-04T08:48:34Z

src/language-js/print/array.js

@@ -170,6 +171,18 @@ function isConciselyPrintedArray(node, options) {
  );
 }

+function isLineAfterElementEmpty(path, { originalText: text }) {


Can we use destructuring assignment here?

Suggested change

function isLineAfterElementEmpty(path, { originalText: text }) {

function isLineAfterElementEmpty({ node }, { originalText: text }) {

fisker · 2023-05-04T08:57:21Z

src/language-js/print/array.js

@@ -9,17 +9,18 @@ import {
  fill,
 } from "../../document/builders.js";
 import hasNewline from "../../utils/has-newline.js";
+import isNextLineEmptyAfterIndex from "../../utils/is-next-line-empty.js";
+import { skipInlineComment, skipTrailingComment } from "../../utils/public.js";


Missed this on previous review. Do not import from public.js, please import from the actual file directly.

solarized-fox · 2023-05-05T04:23:26Z

I've implemented the suggested changes!

solarized-fox · 2023-05-05T05:49:05Z

oops, forgot a semicolon I guess. it's fixed now

Co-authored-by: ☀️ <sol@solfox.online> Co-authored-by: fisker Cheung <lionkay@gmail.com>

github-actions bot mentioned this pull request Apr 21, 2023

Misc tests prettier/prettier-regression-testing#342

Open

fisker self-requested a review April 21, 2023 12:22

solarized-fox force-pushed the #13017 branch from 15aecc0 to df3ad47 Compare April 22, 2023 18:03

solarized-fox force-pushed the #13017 branch from df3ad47 to 8022b18 Compare April 23, 2023 07:27

fisker reviewed Apr 23, 2023

View reviewed changes

src/language-js/print/array.js Outdated Show resolved Hide resolved

solarized-fox force-pushed the #13017 branch from 8022b18 to 0d954d1 Compare April 23, 2023 14:24

☀️ added 4 commits April 24, 2023 00:09

Newlines in arrays are now preserved even when there are characters b…

6f8bb7a

…etween the element and the comma

Add tests

8419147

Update existing snapshots

9e05494

Add changelog

727d2fa

solarized-fox force-pushed the #13017 branch from 0d954d1 to 727d2fa Compare April 24, 2023 04:20

fisker reviewed Apr 24, 2023

View reviewed changes

src/language-js/print/array.js Show resolved Hide resolved

Update changelog

d411464

fisker changed the title ~~Fix trailing parens tripping up skipToLineEnd~~ Fix empty line check between array elements Apr 24, 2023

fisker approved these changes Apr 24, 2023

View reviewed changes

solarized-fox force-pushed the #13017 branch from d148a81 to d411464 Compare April 25, 2023 18:39

sosukesuzuki approved these changes May 4, 2023

View reviewed changes

fisker requested changes May 4, 2023

View reviewed changes

Use destructuring assignment

5db9ab3

Import functions directly

613145e

solarized-fox force-pushed the #13017 branch from d1e8856 to 613145e Compare May 5, 2023 05:48

fisker approved these changes May 5, 2023

View reviewed changes

fisker merged commit 8108864 into prettier:main May 5, 2023
29 checks passed

solarized-fox deleted the #13017 branch May 5, 2023 08:00

medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 15, 2024

Fix empty line check between array elements (prettier#14736)

9858454

Co-authored-by: ☀️ <sol@solfox.online> Co-authored-by: fisker Cheung <lionkay@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix empty line check between array elements #14736

Fix empty line check between array elements #14736

solarized-fox commented Apr 21, 2023 •

edited

fisker commented Apr 21, 2023 •

edited

fisker commented Apr 21, 2023 •

edited

solarized-fox commented Apr 21, 2023

solarized-fox commented Apr 22, 2023

solarized-fox commented Apr 23, 2023

fisker Apr 23, 2023

solarized-fox Apr 23, 2023

solarized-fox commented Apr 23, 2023 •

edited

solarized-fox commented Apr 24, 2023 •

edited

fisker commented Apr 24, 2023 •

edited

solarized-fox commented Apr 24, 2023

fisker commented Apr 24, 2023

solarized-fox commented Apr 24, 2023

solarized-fox commented Apr 24, 2023

fisker left a comment

solarized-fox commented Apr 25, 2023 •

edited

sosukesuzuki left a comment

sosukesuzuki May 4, 2023

fisker May 4, 2023

solarized-fox commented May 5, 2023

solarized-fox commented May 5, 2023 •

edited

	function isLineAfterElementEmpty(path, { originalText: text }) {
	function isLineAfterElementEmpty({ node }, { originalText: text }) {

Fix empty line check between array elements #14736

Fix empty line check between array elements #14736

Conversation

solarized-fox commented Apr 21, 2023 • edited

Description

Checklist

fisker commented Apr 21, 2023 • edited

fisker commented Apr 21, 2023 • edited

solarized-fox commented Apr 21, 2023

solarized-fox commented Apr 22, 2023

solarized-fox commented Apr 23, 2023

fisker Apr 23, 2023

Choose a reason for hiding this comment

solarized-fox Apr 23, 2023

Choose a reason for hiding this comment

solarized-fox commented Apr 23, 2023 • edited

solarized-fox commented Apr 24, 2023 • edited

fisker commented Apr 24, 2023 • edited

solarized-fox commented Apr 24, 2023

fisker commented Apr 24, 2023

solarized-fox commented Apr 24, 2023

solarized-fox commented Apr 24, 2023

fisker left a comment

Choose a reason for hiding this comment

solarized-fox commented Apr 25, 2023 • edited

sosukesuzuki left a comment

Choose a reason for hiding this comment

sosukesuzuki May 4, 2023

Choose a reason for hiding this comment

fisker May 4, 2023

Choose a reason for hiding this comment

solarized-fox commented May 5, 2023

solarized-fox commented May 5, 2023 • edited

solarized-fox commented Apr 21, 2023 •

edited

fisker commented Apr 21, 2023 •

edited

fisker commented Apr 21, 2023 •

edited

solarized-fox commented Apr 23, 2023 •

edited

solarized-fox commented Apr 24, 2023 •

edited

fisker commented Apr 24, 2023 •

edited

solarized-fox commented Apr 25, 2023 •

edited

solarized-fox commented May 5, 2023 •

edited