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

Add transformations for placeholders #51621

Merged
merged 8 commits into from
Jun 20, 2018

Conversation

go2sh
Copy link
Contributor

@go2sh 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
Copy link

msftclas commented Jun 11, 2018

CLA assistant check
All CLA requirements met.

@go2sh
Copy link
Contributor Author

go2sh commented Jun 11, 2018

CC @jrieken

@menendezau
Copy link

Nice work!

@jrieken
Copy link
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;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@go2sh go2sh Jun 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@go2sh
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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 jrieken added the feature-request Request for new features or functionality label Jun 20, 2018
@jrieken
Copy link
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
Copy link
Contributor Author

go2sh commented Jun 20, 2018

@jrieken Done. :)

@jrieken
Copy link
Member

jrieken commented Jun 20, 2018

Thanks.

@jrieken jrieken merged commit 5e1ad2b into microsoft:master Jun 20, 2018
@jrieken
Copy link
Member

jrieken commented Jun 20, 2018

Merged! 🍻 @go2sh

rbost added a commit to rbost/latex-snippets that referenced this pull request Jun 27, 2018
Waiting for the VSCode PR microsoft/vscode#51621 to be released
so we can use more complex snippets.
@beausmith
Copy link

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
Copy link

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
Copy link

csaar95 commented Jul 11, 2018

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

@thorn0
Copy link

thorn0 commented Aug 17, 2018

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

@go2sh
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

thorn0 commented Aug 17, 2018

Ah, now I see it.

@savetheclocktower savetheclocktower mentioned this pull request Jun 23, 2019
15 tasks
@tecosaur
Copy link

I'm trying a simple "$1 ${1/(.*)/${1:/upcase}/}" snippet but it isn't working for me (v1.40.0), has placeholder transform functionality just been completely removed?

@mib32
Copy link

mib32 commented Jan 12, 2020

I'm trying a simple "$1 ${1/(.*)/${1:/upcase}/}" snippet but it isn't working for me (v1.40.0), has placeholder transform functionality just been completely removed?

Was struggling with this for a while, then understood - the transformation is not applied in real time - you have to hit Tab, and then it will get applied. Funny!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality on-testplan snippets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snippet transformations for placeholders
10 participants