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

Snippet wrap #296

Merged
merged 35 commits into from
Oct 25, 2021
Merged

Snippet wrap #296

merged 35 commits into from
Oct 25, 2021

Conversation

pokey
Copy link
Member

@pokey pokey commented Oct 15, 2021

tryWrapFine

Closes #21

  • Switch to cursorless.wrappers.scopeTypes for settings
  • Switch to use language-specific settings instead of language mapping to simplify implementation and make it easier to configure
  • Allow snippets to signal their preferred scope type
  • Support user snippets? If so, we'd prob also want a way to signal what their preferred scope type is
  • Return that mark

@pokey
Copy link
Member Author

pokey commented Oct 15, 2021

@AndreasArvidsson looks like this is failing because the snippet inserts \r\n on windows. Any ideas how to fix? Should we set line ending for all of our tests unless they have a special flag in the yaml?

@pokey pokey marked this pull request as draft October 15, 2021 18:45
@pokey pokey removed the request for review from AndreasArvidsson October 15, 2021 18:45
@pokey pokey marked this pull request as ready for review October 17, 2021 17:34
package.json Outdated
@@ -400,9 +400,100 @@
"verticalOffset": 0
}
}
},
"cursorless.wrapperSnippetPreferences": {
"description": "Allows snippets to signal what their default scope types etcetera should be",
Copy link
Member Author

Choose a reason for hiding this comment

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

Down the road we may allow signalling things like selectionType, etc, so I figured let's have this be a map

package.json Outdated
"properties": {
"scopeType": {
"type": "string",
"enum": [
Copy link
Member Author

Choose a reason for hiding this comment

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

This enum will get stale, but I don't think it will cause any serious problems, and the autocomplete will be nice

@@ -0,0 +1,26 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to duplicate these snippets between languages as I'd imagine they'll start to diverge. but easy to just use one file as well

@@ -31,7 +31,7 @@ interface MarkEntry {
}

class BringMoveSwap implements Action {
targetPreferences: ActionPreferences[] = [
getTargetPreferences: () => ActionPreferences[] = () => [
Copy link
Member Author

Choose a reason for hiding this comment

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

We now have a function to give action a chance to see args


export default class WrapWithSnippet implements Action {
getTargetPreferences(
partialTargets: PartialTarget[],
Copy link
Member Author

Choose a reason for hiding this comment

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

We have this arg here so that below FIXME can be implemented in the future. Mixed feelings about letting action see this thing but not sure how else to let it do something like figure out which editor a target is referring to

Copy link
Member Author

Choose a reason for hiding this comment

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

  • pass in context object with callbacks instead

@@ -0,0 +1,47 @@
spokenForm: try catch wrap this
Copy link
Member Author

Choose a reason for hiding this comment

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

Test for multiple cursors

@@ -59,6 +60,12 @@ async function runTest(file: string) {
});
const editor = await vscode.window.showTextDocument(document);

if (!fixture.initialState.documentContents.includes("\n")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed reasonable to me and doesn't break your line ending tests

);
if (process.env.CURSORLESS_TEST_UPDATE_FIXTURES == "true") {
const outputFixture = { ...fixture, finalState: resultState, returnValue };
await fsp.writeFile(file, serialize(outputFixture));
Copy link
Member Author

Choose a reason for hiding this comment

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

Easy way to update snapshots

@@ -36,6 +36,25 @@
"${workspaceFolder}/out/test/**/*.js"
],
"preLaunchTask": "${defaultBuildTask}"
},
{
"name": "Update fixtures",
Copy link
Member Author

Choose a reason for hiding this comment

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

Easy way to update snapshots

@pokey pokey force-pushed the snippet-wrap branch 2 times, most recently from a0d5e21 to b492207 Compare October 19, 2021 16:55
Copy link
Member Author

@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.

  • remove prefixes so just ifStatement
  • double-check that can say "if wrap token air"

package.json Outdated
@@ -400,9 +400,100 @@
"verticalOffset": 0
}
}
},
"cursorless.wrapperSnippetPreferences": {
Copy link
Member Author

Choose a reason for hiding this comment

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

  • have a cursorless.snippets setting with just a defaultScopeType property.

@@ -7,7 +7,7 @@ import {
} from "../typings/Types";

export class Sort implements Action {
targetPreferences: ActionPreferences[] = [{ insideOutsideType: "inside" }];
getTargetPreferences: () => ActionPreferences[] = () => [{ insideOutsideType: "inside" }];
Copy link
Member Author

Choose a reason for hiding this comment

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

  • see if i can get rid of type here


export default class WrapWithSnippet implements Action {
getTargetPreferences(
partialTargets: PartialTarget[],
Copy link
Member Author

Choose a reason for hiding this comment

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

  • pass in context object with callbacks instead

package.json Outdated
Comment on lines 479 to 514
"ifElseStatement": {
"definitions": [
{
"scope": {
"langIds": [
"typescript",
"typescriptreact",
"javascript",
"javascriptreact",
"cpp",
"c",
"java",
"csharp"
]
},
"body": [
"if ($condition) {\n\t$consequence\n} else {\n\t$alternative\n}"
]
},
{
"scope": {
"langIds": [
"python"
]
},
"body": [
"if $condition:\n\t$consequence\nelse:\n\t$alternative"
]
}
],
"defaultScopeTypes": {
"consequence": "statement",
"alternative": "statement"
},
"description": "If else statement"
}
Copy link
Member Author

@pokey pokey Oct 21, 2021

Choose a reason for hiding this comment

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

@AndreasArvidsson wdyt? See how we have a single snippet for if / else?

package.json Outdated
"type": "string"
}
},
"scopeType": {
Copy link
Member Author

Choose a reason for hiding this comment

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

This scopeType isn't yet supported, but will allow us to differentiate "snip funk" in a class vs top-level in the future, as discussed

package.json Outdated
@@ -400,6 +400,216 @@
"verticalOffset": 0
}
}
},
"cursorless.experimental.snippets": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tagged the setting as experimental

package.json Outdated
"type": "object",
"default": {
"ifStatement": {
"definitions": [],
Copy link
Member Author

Choose a reason for hiding this comment

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

We put the definitions elsewhere so that the user can easily put their own, which take precedence over built-in ones

@@ -115,15 +115,18 @@ function inferPrimitiveTarget(
"contents";

const selectionType =
maybeSelectionType ?? actionPreferences.selectionType ?? "token";
maybeSelectionType ??
(target.modifier == null ? actionPreferences.selectionType : null) ??
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically a hack until we merge scopeType and selectionType using pipeline support

@@ -0,0 +1,436 @@
/*---------------------------------------------------------------------------------------------
Copy link
Member Author

Choose a reason for hiding this comment

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

We vendor in vscode snippet parser. I don't feel great about it. But I think it should be ok as snippet syntax shouldn't change too fast

],
"jsonValidation": [
{
"fileMatch": "*.cursorless-snippets",
Copy link
Member Author

Choose a reason for hiding this comment

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

This extension is a bit verbose; open to suggestions. Need some way to indicate that the file should be checked by the schema on the next line so that user gets autocomplete

@@ -432,12 +450,12 @@
"js-yaml": "^4.1.0",
"mocha": "^8.1.3",
"sinon": "^11.1.1",
"typescript": "^4.1.2",
"typescript": "^4.4.4",
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the typescript version bump

@@ -280,6 +285,9 @@ export async function activate(context: vscode.ExtensionContext) {
thatMark,
sourceMark,
addDecorations,
experimental: {
registerThirdPartySnippets: graph.snippets.registerThirdPartySnippets,
Copy link
Member Author

Choose a reason for hiding this comment

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

Seemed super easy to support third-party snippets so I just decided implement it

this.registerThirdPartySnippets =
this.registerThirdPartySnippets.bind(this);

const timer = setInterval(
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we just poll the user snippet dir, but we check modification times and don't do anything if nothing has changed

@@ -0,0 +1,15 @@
import Actions from "../actions";
Copy link
Member Author

Choose a reason for hiding this comment

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

We use factories now so that we can add the extension context, which is not a class

@pokey pokey merged commit 1633ac1 into main Oct 25, 2021
@pokey pokey deleted the snippet-wrap branch October 25, 2021 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "wrap with snippet"
2 participants