Repository: nicoespeon/abracadabra
Pull request #47 (2019-10-31 17:11:08)

Improve Move Statements on one-liners
- [x] Update Changelog


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cc339409..2e32c620 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,58 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+#### Move Statements now handles one-liners more intuitively
+
+Consider following code:
+
+```js
+const user = {
+  firstName: "John",
+  lastName: "Doe",
+  age: 24
+};
+const rights = { admin: false, moderator: true };
+```
+
+Before, if you had the cursor on `admin` and you tried to move the statement up, you'd have swapped the parameters:
+
+```js
+const user = {
+  firstName: "John",
+  lastName: "Doe",
+  age: 24
+};
+const rights = { moderator: true, admin: false };
+```
+
+But what you probably intended to do was to swap the two variable declarations. From usage, we think "Move Statements" more as something you'd like to use to move things up or down. If things are at the same line, you certainly don't expect them to move.
+
+The behaviour was surprising, so it was improved. Now, the same operation will generate:
+
+```js
+const rights = { admin: false, moderator: true };
+const user = {
+  firstName: "John",
+  lastName: "Doe",
+  age: 24
+};
+```
+
+However, if your cursor is now on `lastName` and you move the statement down, it will still produce the following code since statements are on different lines:
+
+```js
+const rights = { admin: false, moderator: true };
+const user = {
+  firstName: "John",
+  age: 24,
+  lastName: "Doe"
+};
+```
+
+Noticed how it handles the trailing comma? Ok, that was already here. But it's still super neat!
+
 ## [0.10.0] - 2019-10-27
 
 ### Changed
diff --git a/src/editor/selection.ts b/src/editor/selection.ts
index ff5fad53..aecb203b 100644
--- a/src/editor/selection.ts
+++ b/src/editor/selection.ts
@@ -111,4 +111,11 @@ class Selection {
       this.start.isEqualTo(selection.start) && this.end.isEqualTo(selection.end)
     );
   }
+
+  isSameLineThan(selection: Selection): boolean {
+    return (
+      this.start.isSameLineThan(selection.start) &&
+      this.end.isSameLineThan(selection.end)
+    );
+  }
 }
diff --git a/src/refactorings/move-statement-down/move-statement-down.ts b/src/refactorings/move-statement-down/move-statement-down.ts
index a64ee140..322ed999 100644
--- a/src/refactorings/move-statement-down/move-statement-down.ts
+++ b/src/refactorings/move-statement-down/move-statement-down.ts
@@ -118,6 +118,21 @@ function hasChildWhichMatchesSelection(
     if (isBlockStatementDirectChild(childPath)) return;
     if (!matchesSelection(childPath, selection)) return;
 
+    if (typeof childPath.key !== "number") return;
+    const childPathBelowKey = childPath.key + 1;
+    const container = new Array().concat(childPath.container);
+    const hasPathBelow = childPathBelowKey < container.length;
+    if (!hasPathBelow) return;
+
+    const { node: childNode } = childPath;
+    const { node: childBelowNode } = childPath.getSibling(childPathBelowKey);
+    if (!ast.isSelectableNode(childBelowNode)) return;
+    if (!ast.isSelectableNode(childNode)) return;
+
+    const nodeSelection = Selection.fromAST(childNode.loc);
+    const nodeBelowSelection = Selection.fromAST(childBelowNode.loc);
+    if (nodeSelection.isSameLineThan(nodeBelowSelection)) return;
+
     result = true;
     childPath.stop();
   }
diff --git a/src/refactorings/move-statement-up/move-statement-up.ts b/src/refactorings/move-statement-up/move-statement-up.ts
index 67811b34..3c014be0 100644
--- a/src/refactorings/move-statement-up/move-statement-up.ts
+++ b/src/refactorings/move-statement-up/move-statement-up.ts
@@ -101,6 +101,19 @@ function hasChildWhichMatchesSelection(
     if (isBlockStatementDirectChild(childPath)) return;
     if (!matchesSelection(childPath, selection)) return;
 
+    if (typeof childPath.key !== "number") return;
+    const childPathAboveKey = childPath.key - 1;
+    if (childPathAboveKey < 0) return;
+
+    const { node: childNode } = childPath;
+    const { node: childNodeAbove } = childPath.getSibling(childPathAboveKey);
+    if (!ast.isSelectableNode(childNodeAbove)) return;
+    if (!ast.isSelectableNode(childNode)) return;
+
+    const nodeSelection = Selection.fromAST(childNode.loc);
+    const nodeAboveSelection = Selection.fromAST(childNodeAbove.loc);
+    if (nodeSelection.isSameLineThan(nodeAboveSelection)) return;
+
     result = true;
     childPath.stop();
   }


Test patch:
diff --git a/src/refactorings/move-statement-down/move-statement-down.test.ts b/src/refactorings/move-statement-down/move-statement-down.test.ts
index f077379a..7c430455 100644
--- a/src/refactorings/move-statement-down/move-statement-down.test.ts
+++ b/src/refactorings/move-statement-down/move-statement-down.test.ts
@@ -197,6 +197,15 @@ console.log("Should not move");`,
 console.log("Should not move");`,
         expectedPosition: new Position(2, 2)
       },
+      {
+        description: "object properties, one-liner",
+        code: `const data = { foo: "foo", bar: "bar" };
+console.log("Should move in this scenario");`,
+        selection: Selection.cursorAt(0, 16),
+        expected: `console.log("Should move in this scenario");
+const data = { foo: "foo", bar: "bar" };`,
+        expectedPosition: new Position(1, 16)
+      },
       {
         description: "object properties, cursor after comma",
         code: `const data = {
diff --git a/src/refactorings/move-statement-up/move-statement-up.test.ts b/src/refactorings/move-statement-up/move-statement-up.test.ts
index b6e78774..d2cf0087 100644
--- a/src/refactorings/move-statement-up/move-statement-up.test.ts
+++ b/src/refactorings/move-statement-up/move-statement-up.test.ts
@@ -150,6 +150,15 @@ const data = {
 };`,
         expectedPosition: new Position(2, 2)
       },
+      {
+        description: "object properties, one-liner",
+        code: `console.log("Should move in this scenario");
+const data = { foo: "foo", bar: "bar" };`,
+        selection: Selection.cursorAt(1, 16),
+        expected: `const data = { foo: "foo", bar: "bar" };
+console.log("Should move in this scenario");`,
+        expectedPosition: new Position(0, 16)
+      },
       {
         description: "object properties, cursor after comma",
         code: `console.log("Should not move");