Skip to content

Commit

Permalink
Added tests, and fixed implementation
Browse files Browse the repository at this point in the history
 - Still missing refactor, but its getting somewhere.
  • Loading branch information
Strepto committed Mar 2, 2021
1 parent 2aaf087 commit 96eb365
Show file tree
Hide file tree
Showing 2 changed files with 306 additions and 17 deletions.
83 changes: 69 additions & 14 deletions src/providers/codeAction/swapListItemCodeAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
} from "../codeActionProvider";
import { ICodeActionParams } from "../paramsExtensions";

// Handle adding annotation to let expr declaration
const refactorName = "swap_listitem";
const moveListItemUpActionName = "swap_listitem_up";
const moveListItemDownActionName = "swap_listitem_down";
Expand All @@ -25,14 +24,17 @@ CodeActionProvider.registerRefactorAction(refactorName, {
params.range.start,
);

const isListValid = isInValidList(nodeAtPosition);
if (!isListValid) return [];

const canMoveNext = getTargetNodes(nodeAtPosition, "next");
const canMovePrev = getTargetNodes(nodeAtPosition, "previous");

const codeActions: IRefactorCodeAction[] = [];

if (canMovePrev) {
codeActions.push({
title: "Move List item Up",
title: "Move list item up",
kind: CodeActionKind.RefactorRewrite + ".movelistitem.up",
data: {
actionName: moveListItemUpActionName,
Expand All @@ -45,7 +47,7 @@ CodeActionProvider.registerRefactorAction(refactorName, {

if (canMoveNext) {
codeActions.push({
title: "Move List item Down",
title: "Move list item down",
kind: CodeActionKind.RefactorRewrite + ".movelistitem.down",
data: {
actionName: moveListItemDownActionName,
Expand Down Expand Up @@ -105,21 +107,41 @@ function getEdits(

const nodeToMove = targets.nodeToMove;
const nodeToSwapWith = targets.nodeToSwapWith;
const nodeToMoveText = nodeToMove.text;
const nodeToSwapText = nodeToSwapWith.text;

const start = findListItemBoundNode(nodeToMove, "previous");

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

These need renaming. I would like something like nodeToMovePreviousListSeparator, but feel free to find something better

const end = findListItemBoundNode(nodeToMove, "next");
const start2 = findListItemBoundNode(nodeToSwapWith, "previous");
const end2 = findListItemBoundNode(nodeToSwapWith, "next");

if (!(start && end && start2 && end2)) return [];

// let nodeToMoveText = "";
// nodes1.forEach((x) => (nodeToMoveText += x.text));

// let nodeToSwapText = "";
// nodes2.forEach((x) => (nodeToSwapText += x.text));

const nodeToMoveText = params.sourceFile.tree.rootNode.text.substring(
start.endIndex,
end.startIndex,
);

const nodeToSwapText = params.sourceFile.tree.rootNode.text.substring(

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

Should be nodeToSwapWithText. I think the With is important for context

start2.endIndex,
end2.startIndex,
);
const startPosition = PositionUtil.FROM_TS_POSITION(
nodeToMove.startPosition,
start?.endPosition,
).toVSPosition();
const endPosition = PositionUtil.FROM_TS_POSITION(
nodeToMove.endPosition,
end.startPosition,
).toVSPosition();

const startPosition2 = PositionUtil.FROM_TS_POSITION(
nodeToSwapWith.startPosition,
start2.endPosition,
).toVSPosition();
const endPosition2 = PositionUtil.FROM_TS_POSITION(
nodeToSwapWith.endPosition,
end2.startPosition,
).toVSPosition();

return [
Expand All @@ -143,11 +165,12 @@ function findSiblingSemanticListNode(

let target = iterate(node);
while (
(target?.type == "," ||
target?.type == "line_comment" ||
target?.type == "[" ||
target?.type == "]") &&
target != null
target != null &&
(target.type == "," ||
target.type == "line_comment" ||
target.type == "block_comment" ||
target.type == "[" ||
target.type == "]")
) {
if (target.type == "[" || target.type == "]") return null;
target = iterate(target);
Expand All @@ -159,3 +182,35 @@ function findSiblingSemanticListNode(
return isNext ? inputNode.nextSibling : inputNode.previousSibling;
}
}

function isInValidList(node: SyntaxNode): boolean {
const list = TreeUtils.findParentOfType("list_expr", node);
if (!list) return false;

const listText = list.text.trim();
return listText.startsWith("[") && listText.endsWith("]");

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

I'm just wondering, is there value in this? Or can this be omitted?

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

Okay, it's to not work on unfinished lists I assume, as tree-sitter might fix the ast for a missing icon (but with an error)

}

// Find the next list item bound node ('[' or ',' or ']')
function findListItemBoundNode(

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

How about findCommaOrListBrackets? I think even findNextCommaOrListBrackets and findPreviousCommaOrListBrackets would be nicer than that, but would depend on you either doing another wrapper layer or (almost) duplicating the code. Both would be fine in my book.

node: SyntaxNode,
direction: "previous" | "next",
): SyntaxNode | null {
const isNext = direction == "next";

let target = iterate(node);
while (
target != null &&
target.type != "," &&
target.type != "[" &&
target.type != "]"
) {
target = iterate(target);
}

return target;

function iterate(inputNode: SyntaxNode): SyntaxNode | null {
return isNext ? inputNode.nextSibling : inputNode.previousSibling;
}
}
240 changes: 237 additions & 3 deletions test/codeActionTests/swapListItemCodeAction.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { testCodeAction } from "./codeActionTestBase";

describe("swap list item code action", () => {
it("should swap item aa with bb", async () => {
it("should swap item aa with bb in horizontal list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
Expand All @@ -23,8 +23,242 @@ func =

await testCodeAction(
source,
[{ title: "Move List item Down" }],
[{ title: "Move list item down" }],
expectedSource,
);
});
})

it("should swap item aa with bb in vertical list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ "aa"

This comment has been minimized.

Copy link
@razzeee

razzeee Mar 2, 2021

Member

Small nitpick, it might be nice to have test cases, where both items don't have the same length.

--^
, "bb"
, "cc"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bb"
, "aa"
, "cc"
]
`;

await testCodeAction(
source,
[{ title: "Move list item down" }],
expectedSource,
);
});
it("should swap item aa with bb in non-standard formatted list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ "a", "bbb",
--^
"cc"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bbb", "a",
"cc"
]
`;

await testCodeAction(
source,
[{ title: "Move list item down" }],
expectedSource,
);
});

it("should swap item aa with bb in vertical list upwards", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ -- Comment
let
a = 1
in
"aa"
, "bb"
--^
, "cc"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bb"
, -- Comment
let
a = 1
in
"aa"
, "cc"
]
`;

await testCodeAction(
source,
[{ title: "Move list item up" }],
expectedSource,
);
});

it("should move comment along with item in vertical list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ -- CommentAA
"aa"
--^
, "bb"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bb"
, -- CommentAA
"aa"
]
`;

await testCodeAction(
source,
[{ title: "Move list item down" }],
expectedSource,
);
});

it("should move block_comment along with item in vertical list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ {-| CommentAA
-}
"aa"
--^
, "bb"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bb"
, {-| CommentAA
-}
"aa"
]
`;

await testCodeAction(
source,
[{ title: "Move list item down" }],
expectedSource,
);
});

it("should NOT move comment in preceding item along with item in vertical list", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
[ "aa"
--^
-- "bb"
, "bb"
]
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
[ "bb"
, "aa"
-- "bb"
]
`;

await testCodeAction(
source,
[{ title: "Move list item down" }],
expectedSource,
);
});

it("should not handle unfinished lists?", async () => {
const source = `
--@ Test.elm
module Test exposing (..)
func =
([ "aa"
--^
, "bb"
)
`;

const expectedSource = `
--@ Test.elm
module Test exposing (..)
func =
([ "bb"
, "aa"
)
`;

// await testCodeAction(source, [], expectedSource);

This comment has been minimized.

Copy link
@Strepto

Strepto Mar 3, 2021

Author

I would like to test some cases where a CodeAction should NOT be available. Such as "Move list item up" not being available on the first item. And in this particular test no Move list item should appear.)

Does this seem like something I/we could extend the testCodeAction code with @razzeee ?

});
});

0 comments on commit 96eb365

Please sign in to comment.