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

Add transformations for placeholders #51621

Merged
merged 8 commits into from Jun 20, 2018

Conversation

Projects
None yet
8 participants
@go2sh
Contributor

go2sh commented Jun 11, 2018

This PR adds transformations for placeholders by extending the syntax in the same way transformations are use with variables. Transformations are executed, when the user changes to the next placeholder.

I don't know if there is a better/more efficient way, but for me it works.

Fixes #34683.
TODO:

  • Add tests
  • Update documentation

PS: My first contribution to vscode. Great editor. :)
Best Regards,
go2sh

@msftclas

This comment has been minimized.

msftclas commented Jun 11, 2018

CLA assistant check
All CLA requirements met.

@go2sh

This comment has been minimized.

Contributor

go2sh commented Jun 11, 2018

@menendezau

This comment has been minimized.

menendezau commented Jun 11, 2018

Nice work!

@jrieken jrieken added the snippets label Jun 12, 2018

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 12, 2018

👏 looks good so far, keep going esp. with adding tests. Check out snippetParser.test.ts and snippetSession.test.ts. Left a few questions about the grammar/rule-of-transform as comments

if (this._parseTransform(placeholder)) {
parent.appendChild(placeholder);
return true;
}

This comment has been minimized.

@jrieken

jrieken Jun 12, 2018

Member

Not sure if that's actually supported by TextMate. Do you have a reference?

This comment has been minimized.

@go2sh

go2sh Jun 12, 2018

Contributor

As far as I know, TextMate only supports transformations for the simple case ${1/.../.../...}. This two here would be an extension to the TextMate behavior. I agree, that the choice case might not be needed, but the nested one would be nice for default values, which covers exactly my use case. I implemented transformations for all case to have a consistent behavior.

This comment has been minimized.

@go2sh
@go2sh

This comment has been minimized.

Contributor

go2sh commented Jun 12, 2018

Another thing that differs from the TextMate behavior is that only the first placeholder is used for the transformation. I'll change that as well, so that every placeholder can holde it's own transformation.

@go2sh

This comment has been minimized.

Contributor

go2sh commented Jun 15, 2018

@jrieken I fixed the code and added quite some tests. I hope this is enough for merging. ;-)

Now VS Code supports:

${1/.../.../...}
${1:.../.../.../...}
${1|...|/.../.../...}

Which is a super set of the TextMate Syntax. So tm snippets work in vscode, but extended vscode snippets don't work in tm. Every placeholder has its one transformation, so you can create two different results for the same index.

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 18, 2018

Thanks - quite busy the next few days but my hope is to squeeze this in during this week...

@go2sh

This comment has been minimized.

Contributor

go2sh commented Jun 18, 2018

Would be nice to see it in the next Update. I use this feature a lot. :-)

@jrieken jrieken added this to the June 2018 milestone Jun 20, 2018

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 20, 2018

Bang on. Thanks! Two small things left:

  • please add a test in snippetSession.test that uses two (or more) selection at different indentation. Like test('snippets, transform'... but with multiple selection. I test this manually but it would be best to have tested too.
  • update snippet.md with a placeholder transform section and please outline what isn't standard TM.

Scheduled this now for June, let's ship this.

@go2sh

This comment has been minimized.

Contributor

go2sh commented Jun 20, 2018

@jrieken Done. :)

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 20, 2018

Thanks.

@jrieken jrieken merged commit 5e1ad2b into Microsoft:master Jun 20, 2018

2 checks passed

VSTS: VS Code 20180620.59 succeeded
Details
license/cla All CLA requirements met.
Details
@jrieken

This comment has been minimized.

Member

jrieken commented Jun 20, 2018

Merged! 🍻 @go2sh

@jrieken jrieken added the on-testplan label Jun 25, 2018

rbost added a commit to rbost/latex-snippets that referenced this pull request Jun 27, 2018

Small fixes to make the snippets usable.
Waiting for the VSCode PR Microsoft/vscode#51621 to be released
so we can use more complex snippets.
@beausmith

This comment has been minimized.

beausmith commented Jul 10, 2018

How to use /upcase, /downcase/, and /capitalize?

I've read the docs and tried:

"test": {
    "prefix": "test",
    "body": "${1} -> ${1:/upcase} ${1:/downcase}"
},

and

"test": {
    "prefix": "test",
    "body": "${1} -> ${1/upcase} ${1/downcase}"
},

…but neither work.

I don't see any tests for these transforms in this PR.

I'm stumped. I posted a question here:
https://stackoverflow.com/questions/51272365/vs-code-how-to-convert-snippet-placeholder-to-uppercase-or-lowercase

cc @go2sh

@moranje

This comment has been minimized.

moranje commented Jul 10, 2018

The integer in the EBNF docs refers to a RegExp group not to a tabstop reference so should work when typing Asdf:

    "test": {
        "prefix": "test",
        "body": "${1} -> ${1/(Asdf)/${1:/upcase}/} ${1/(Asdf)/${1:/downcase}/}"
    }
@csaar95

This comment has been minimized.

csaar95 commented Jul 11, 2018

In addition to #34683, it also doesn't support choice: "${1|hello,world|} -> ${1/hello/hallo/}"

@thorn0

This comment has been minimized.

thorn0 commented Aug 17, 2018

Can't make ${1:.../.../.../...} work in 1.27.0-insiders.

@go2sh

This comment has been minimized.

Contributor

go2sh commented Aug 17, 2018

@beausmith @moranje @csaar95 @thorn0 The vscode team removed all the advanced options as they create some regressions on existing snippets. See the docs. Only ${1/../../} is supported. Sorry for that. The removal also broke my usecase...

@thorn0

This comment has been minimized.

thorn0 commented Aug 17, 2018

The docs aren't clear about this. The Grammar section doesn't include placeholder transforms at all. At the same time, the Placeholder Transform section doesn't have any examples and doesn't mention whether transforms can be combined with default values. So it's not really clear what supposed to be supported and what is not.

@go2sh

This comment has been minimized.

Contributor

go2sh commented Aug 17, 2018

The grammar is clear. Transform is only valid for a tab-stop or a variable. The docs are not perfect. I try to make a pr. The Placeholder Transform section is from a time, where every thing was possible.

@thorn0

This comment has been minimized.

thorn0 commented Aug 17, 2018

Ah, now I see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment