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 transforms do not handle regex with alternatives or optional matches #36089

Closed
dogoku opened this issue Oct 11, 2017 · 14 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug snippets verified Verification succeeded
Milestone

Comments

@dogoku
Copy link

dogoku commented Oct 11, 2017

  • VSCode Version: v1.17
  • OS Version: N/A

Snippet transform functions (/upcase, /downcase, /capitalize) do not apply to optional capture groups, such as in case of a regex with alternatives, e.g. the regex in the example below (explanation). Instead the matches get replaced by the literal strings (i.e the string "/upcase").

I had a look at the grammar and it seems that the EBNF does not specify what if and else mean for the format rule, so I guess the implementation is not technically "wrong". But given that your intention for this feature is to be compatible with TM and ST snippets, then I see this as a bug, since this is possible in ST (albeit with a different format syntax).


Steps to Reproduce:

  1. Add the following snippet to your js user snippets
"Class name": {
    "prefix": "hclass",
    "body": "${1:${TM_FILENAME/^(.)|(?:-(.))|(\\.js)/${1:+/upcase}${2:+/upcase}/g}}",
    "description": "Converts a hyphenated filename to a class name"
}
  1. Open a js file with a hyphenated name, e.g my-awesome-module.js
  2. Type hclass and expand the snippet

Expected result: MyAwesomeModule
Actual result: /upcasey/upcasewesome/upcaseodule

@vscodebot vscodebot bot added editor editor-find Editor find operations labels Oct 11, 2017
@jrieken jrieken added snippets and removed editor editor-find Editor find operations labels Oct 12, 2017
@jrieken jrieken self-assigned this Oct 12, 2017
@MartynasZilinskas
Copy link

I was looking how to change from kebab-case to PascalCase too. This would be really nice to have.

@jrieken jrieken added the bug Issue identified by VS Code Team member as probable bug label Oct 13, 2017
@jrieken jrieken added this to the October 2017 milestone Oct 13, 2017
jrieken added a commit that referenced this issue Oct 13, 2017
@jrieken
Copy link
Member

jrieken commented Oct 13, 2017

Yeah, there is something off. It should be ${1:/upcase} and not ${1:+/upcase} but it still won't work... I'll investigate. Maybe using String#replace isn't enough

@dogoku
Copy link
Author

dogoku commented Oct 13, 2017

I used the ${1:+/upcase} because the regex has alternate cases, so the group I am trying to replace might not match each case (it's a global regex, so it will match multiple times). I had to do the same thing for the ST version as well, i.e: (?1\u$1:)(?2\u$2:)

Actually i tried ${1:/upcase} as well, when the first one didn't work, but it throws an exception, cause it cant replace something undefined, which makes sense if you think about it.

@jrieken
Copy link
Member

jrieken commented Oct 13, 2017

Yeah, /upcase et al need to be smarter. And the whole regex evaluation needs to be a little smarter... Thanks for this issue btw, without a real spec it's kinda hard to get this right in the first place. This is spec by sample ;-)

@dogoku
Copy link
Author

dogoku commented Oct 13, 2017

Let me know if I can help in some way. I haven't looked at the code at all, but I'm willing to help out. Thx

sandy081 pushed a commit that referenced this issue Oct 16, 2017
@ashclarke
Copy link

I'm trying to do something similar with CapitalCase filename -> kebab-case.

I have ${TM_FILENAME_BASE/([A-Z][a-z]+)([A-Z][a-z]+$)?/$1-$2/g} but no way to downcase it after.

I tried wrapping it with another variable but get an Invalid Arguments warning.

${templatepath:${TM_FILENAME_BASE/([A-Z][a-z]+)([A-Z][a-z]+$)?/$1-$2/g}/downcase}

and this result:

Capital-Case-Name/downcase

I'm not sure if I've just read the capability of this wrong, or if something is missing in VS Code.

@jrieken
Copy link
Member

jrieken commented Oct 30, 2017

@dogoku I have pushed changes to make the format string smarter (that means not explode when having no match). You can now use this pattern: ${TM_FILENAME/^(.)|(?:-(.))|(\\.js)/${1:/upcase}${2:/upcase}/g}, which is also how it works in Textmate itself.

@ashclarke You need to apply the /downcase function on the match, so ${1:/downcase} etc. Check out: e8f088d

@jrieken jrieken closed this as completed Oct 30, 2017
@ashclarke
Copy link

Thanks @jrieken - I will check it out!

@dogoku
Copy link
Author

dogoku commented Oct 31, 2017

Thanks @jrieken, is this patch available in Insider's build?

@jrieken
Copy link
Member

jrieken commented Oct 31, 2017

Yeah, should be available with todays insiders build

egamma pushed a commit that referenced this issue Oct 31, 2017
@mjbvz mjbvz added the verified Verification succeeded label Nov 2, 2017
@dogoku
Copy link
Author

dogoku commented Nov 5, 2017

@jrieken Sorry to do this again, but I found another bug :(

In the latest insiders build (v1.18.0-86e057cd), transforms are working for optional matches, however not all the transforms are applied if you reused the same variable (e.g $1) in other parts of the snippet.

Given the following simplified example:

//TM_FILENAME_BASE: `my-awesome-module`
[
"function ${1:${TM_FILENAME_BASE/^(.)|-(.)/${1:/upcase}${2:/upcase}/gm}} () {}",
"module.exports = $1;"
]

//Expected
function MyAwesomeModule () {}
module.exports = MyAwesomeModule;

//Actual
function MyAwesomeModule () {}
module.exports = My-awesome-module;

In case you missed it, the second use of $1 was not transformed properly, having only the first transform applied to it ${1:/upcase}, where as the second transform ${2:/upcase} was not applied at all, resulting in the string My-awesome-module instead of MyAwesomeModule

Should I open a new ticket for this?

@ashclarke
Copy link

ashclarke commented Nov 5, 2017 via email

@jrieken
Copy link
Member

jrieken commented Nov 6, 2017

Sorry to do this again, but I found another bug :(

Nothing to be sorry about except for not filing a new issue.

@dogoku
Copy link
Author

dogoku commented Nov 6, 2017

Nothing to be sorry about except for not filing a new issue

No worries @jrieken here you go: #37702

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug snippets verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants