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

Properly handle snippet migration with multiple different snippets in one file and noop phrase #2838

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Feb 7, 2025

Fixes #2836
Fixes #2840

Input

{
  "mobxConstructor": {
    "definitions": [
      {
        "scope": {
          "langIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact"
          ]
        },
        "body": [
          "constructor($parameters) {",
          "\tmakeAutoObservable(this);",
          "}"
        ]
      }
    ],
    "description": "Constructor using makeAutoObservable",
    "insertionScopeTypes": [
      "namedFunction"
    ]
  },
  "constantDeclaration": {
    "definitions": [
      {
        "scope": {
          "langIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact"
          ]
        },
        "body": [
          "const $name = ${value/^([^;]*);?$/$1/};"
        ],
        "variables": {
          "name": {
            "formatter": "camelCase"
          }
        }
      }
    ],
    "description": "Constant variable declaration",
    "insertionScopeTypes": [
      "statement"
    ],
    "variables": {
      "value": {
        "wrapperScopeType": "statement"
      }
    }
  },
  "constantDeclarationWithType": {
    "definitions": [
      {
        "scope": {
          "langIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact"
          ]
        },
        "body": [
          "const $name: $type = ${value/^([^;]*);?$/$1/};"
        ],
        "variables": {
          "name": {
            "formatter": "camelCase"
          }
        }
      }
    ],
    "description": "Constant variable declaration with type",
    "insertionScopeTypes": [
      "statement"
    ],
    "variables": {
      "value": {
        "wrapperScopeType": "statement"
      }
    }
  },
  "letDeclaration": {
    "definitions": [
      {
        "scope": {
          "langIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact"
          ]
        },
        "body": [
          "let $name = ${value/^([^;]*);?$/$1/};"
        ],
        "variables": {
          "name": {
            "formatter": "camelCase"
          }
        }
      }
    ],
    "description": "Let variable declaration",
    "insertionScopeTypes": [
      "statement"
    ],
    "variables": {
      "value": {
        "wrapperScopeType": "statement"
      }
    }
  },
  "letDeclarationWithType": {
    "definitions": [
      {
        "scope": {
          "langIds": [
            "typescript",
            "javascript",
            "typescriptreact",
            "javascriptreact"
          ]
        },
        "body": [
          "let $name: $type = ${value/^([^;]*);?$/$1/};"
        ],
        "variables": {
          "name": {
            "formatter": "camelCase"
          }
        }
      }
    ],
    "description": "Let variable declaration with type",
    "insertionScopeTypes": [
      "statement"
    ],
    "variables": {
      "value": {
        "wrapperScopeType": "statement"
      }
    }
  }
}

Output

name: mobxConstructor
description: Constructor using makeAutoObservable
language: typescript | javascript | typescriptreact | javascriptreact
insertionScope: namedFunction
-
constructor($parameters) {
    makeAutoObservable(this);
}
---

name: constantDeclaration
description: Constant variable declaration
language: typescript | javascript | typescriptreact | javascriptreact
insertionScope: statement

$name.insertionFormatter: PRIVATE_CAMEL_CASE
$value.wrapperScope: statement
-
const $name = ${value/^([^;]*);?$/$1/};
---

name: constantDeclarationWithType
description: Constant variable declaration with type
language: typescript | javascript | typescriptreact | javascriptreact
insertionScope: statement

$name.insertionFormatter: PRIVATE_CAMEL_CASE
$value.wrapperScope: statement
-
const $name: $type = ${value/^([^;]*);?$/$1/};
---

name: letDeclaration
description: Let variable declaration
language: typescript | javascript | typescriptreact | javascriptreact
insertionScope: statement

$name.insertionFormatter: PRIVATE_CAMEL_CASE
$value.wrapperScope: statement
-
let $name = ${value/^([^;]*);?$/$1/};
---

name: letDeclarationWithType
description: Let variable declaration with type
language: typescript | javascript | typescriptreact | javascriptreact
insertionScope: statement

$name.insertionFormatter: PRIVATE_CAMEL_CASE
$value.wrapperScope: statement
-
let $name: $type = ${value/^([^;]*);?$/$1/};
---

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, but it needs a test. Are there no existing tests for snippet migrator? Seems complex enough to justify tests, and easy to write, no? Just input / output fixture and compare output to expected?

@AndreasArvidsson
Copy link
Member Author

This looks fine, but it needs a test. Are there no existing tests for snippet migrator? Seems complex enough to justify tests, and easy to write, no? Just input / output fixture and compare output to expected?

I can write a few tests.

@AndreasArvidsson
Copy link
Member Author

@pokey Tests added

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I can't decide how I feel about the way you've represented the expected outputs. I can see both sides. I would lean towards just using the final output, as we're kind of locked into that given its use by end users, so I'm not sure how much benefit we get from abstracting it away. But I can see the benefit of your representation too and I don't think it's worth the effort to change even if end output is better

Fwiw here is diff from snippet migrator on main for my snippets:

diff --git a/core/snippets/snippets/binder.snippet b/core/snippets/snippets/binder.snippet
index 40f53803..0373dce0 100644
--- a/core/snippets/snippets/binder.snippet
+++ b/core/snippets/snippets/binder.snippet
@@ -2,6 +2,8 @@ name: bindClassFunction
 description: Bind function or arg
 phrase: binder
 insertionScope: statement
+
+$name.wrapperPhrase: binder
 ---
 
 language: typescript | javascript | typescriptreact | javascriptreact
diff --git a/core/snippets/snippets/ifElseStatement.snippet b/core/snippets/snippets/ifElseStatement.snippet
index 04acb670..b6a5c0e2 100644
--- a/core/snippets/snippets/ifElseStatement.snippet
+++ b/core/snippets/snippets/ifElseStatement.snippet
@@ -1,5 +1,8 @@
 name: ifElseStatement
 phrase: if else
+
+$alternative.wrapperPhrase: else
+$consequence.wrapperPhrase: if else
 ---
 
 language: shellscript
diff --git a/core/snippets/snippets/ifStatement_CONFLICT.snippet b/core/snippets/snippets/ifStatement_CONFLICT.snippet
index cb3970b3..c0ff6f58 100644
--- a/core/snippets/snippets/ifStatement_CONFLICT.snippet
+++ b/core/snippets/snippets/ifStatement_CONFLICT.snippet
@@ -3,6 +3,7 @@ phrase: if
 
 $condition.wrapperPhrase: condition
 $condition.wrapperScope: statement
+$consequence.wrapperPhrase: if
 ---
 
 language: typescript | typescriptreact | javascript | javascriptreact | cpp | c | java | csharp
diff --git a/core/snippets/snippets/link.snippet b/core/snippets/snippets/link.snippet
index cdc0b337..e983bf99 100644
--- a/core/snippets/snippets/link.snippet
+++ b/core/snippets/snippets/link.snippet
@@ -1,6 +1,8 @@
 name: link
 description: Link
 phrase: link
+
+$text.wrapperPhrase: link
 ---
 
 language: markdown
diff --git a/core/snippets/snippets/map.snippet b/core/snippets/snippets/map.snippet
index b8c81a34..fe113cf6 100644
--- a/core/snippets/snippets/map.snippet
+++ b/core/snippets/snippets/map.snippet
@@ -1,6 +1,8 @@
 name: map
 description: Map
 phrase: map
+
+$value.wrapperPhrase: map
 ---
 
 language: javascript | typescript | typescriptreact | javascriptreact
diff --git a/core/snippets/snippets/tryCatchStatement_CONFLICT.snippet b/core/snippets/snippets/tryCatchStatement_CONFLICT.snippet
index bb926470..499f2ac1 100644
--- a/core/snippets/snippets/tryCatchStatement_CONFLICT.snippet
+++ b/core/snippets/snippets/tryCatchStatement_CONFLICT.snippet
@@ -1,5 +1,7 @@
 name: tryCatchStatement
 phrase: try
+
+$body.wrapperPhrase: try
 ---
 
 language: typescript | typescriptreact | javascript | javascriptreact
diff --git a/core/snippets/snippets/typescript_CONFLICT.snippet b/core/snippets/snippets/typescript_CONFLICT.snippet
index 57ea5849..b1524873 100644
--- a/core/snippets/snippets/typescript_CONFLICT.snippet
+++ b/core/snippets/snippets/typescript_CONFLICT.snippet
@@ -1,43 +1,61 @@
-name: letDeclarationWithType
-description: Let variable declaration with type
-phrase: let type
-insertionScope: statement
-
-$value.wrapperPhrase: let type
-$value.wrapperScope: statement
----
-
+name: mobxConstructor
+description: Constructor using makeAutoObservable
 language: typescript | javascript | typescriptreact | javascriptreact
+insertionScope: namedFunction
 -
 constructor($parameters) {
     makeAutoObservable(this);
 }
 ---
 
+name: constantDeclaration
+description: Constant variable declaration
 language: typescript | javascript | typescriptreact | javascriptreact
+phrase: con
+insertionScope: statement
 
 $name.insertionFormatter: PRIVATE_CAMEL_CASE
+$value.wrapperPhrase: con
+$value.wrapperScope: statement
 -
 const $name = ${value/^([^;]*);?$/$1/};
 ---
 
+name: constantDeclarationWithType
+description: Constant variable declaration with type
 language: typescript | javascript | typescriptreact | javascriptreact
+phrase: con type
+insertionScope: statement
 
 $name.insertionFormatter: PRIVATE_CAMEL_CASE
+$value.wrapperPhrase: con type
+$value.wrapperScope: statement
 -
 const $name: $type = ${value/^([^;]*);?$/$1/};
 ---
 
+name: letDeclaration
+description: Let variable declaration
 language: typescript | javascript | typescriptreact | javascriptreact
+phrase: let
+insertionScope: statement
 
 $name.insertionFormatter: PRIVATE_CAMEL_CASE
+$value.wrapperPhrase: let
+$value.wrapperScope: statement
 -
 let $name = ${value/^([^;]*);?$/$1/};
 ---
 
+name: letDeclarationWithType
+description: Let variable declaration with type
 language: typescript | javascript | typescriptreact | javascriptreact
+phrase: let type
+insertionScope: statement
 
 $name.insertionFormatter: PRIVATE_CAMEL_CASE
+$value.wrapperPhrase: let type
+$value.wrapperScope: statement
 -
 let $name: $type = ${value/^([^;]*);?$/$1/};
 ---

pokey added a commit to pokey/pokey_talon that referenced this pull request Feb 9, 2025
@AndreasArvidsson AndreasArvidsson added this pull request to the merge queue Feb 10, 2025
@AndreasArvidsson AndreasArvidsson removed this pull request from the merge queue due to a manual request Feb 10, 2025
@AndreasArvidsson
Copy link
Member Author

  • Migration of noop phrases now work
  • Output format updated

@AndreasArvidsson AndreasArvidsson changed the title Properly handle snippet migration with multiple different snippets in one file Properly handle snippet migration with multiple different snippets in one file and noop phrase Feb 10, 2025
@pokey pokey enabled auto-merge February 10, 2025 16:36
@pokey pokey added this pull request to the merge queue Feb 10, 2025
Merged via the queue into main with commit 5d4141c Feb 10, 2025
16 checks passed
@pokey pokey deleted the snippetMigrationMultiple branch February 10, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants