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

[WIP] Cake support #1681

Merged
merged 2 commits into from
Oct 5, 2017
Merged

[WIP] Cake support #1681

merged 2 commits into from
Oct 5, 2017

Conversation

bjorkstromm
Copy link
Contributor

Adding support for Cake (I think). Honestly, I have no idea what I'm doing 😄 @gep13 This needs some TypeScript love from you.

OmniSharp-Roslyn PR here OmniSharp/omnisharp-roslyn#932

NB! For some reason, you'll have to switch language mode from Cake to C# (If you have Cake VS Code Extension installed) to get intellisense.

package.json Outdated
@@ -460,13 +462,11 @@
"razor"
]
},
"runtime": "node",
Copy link
Member

Choose a reason for hiding this comment

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

You don't want this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, are VSCode adding these changes automatically? Can't remember changing these.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this happens when you debug the extension.

package.json Outdated
"runtimeArgs": [],
"variables": {
"pickProcess": "csharp.listProcess",
"pickRemoteProcess": "csharp.listRemoteProcess"
},
"program": "./out/src/coreclr-debug/proxy.js",
Copy link
Member

Choose a reason for hiding this comment

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

Or this one

package.json Outdated
},
"linux": {
"program": "./.debugger/vsdbg-ui"
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Or these

package.json Outdated
@@ -1387,13 +1396,11 @@
"razor"
]
},
"runtime": "node",
Copy link
Member

Choose a reason for hiding this comment

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

Not this

package.json Outdated
"runtimeArgs": [],
"variables": {
"pickProcess": "csharp.listProcess",
"pickRemoteProcess": "csharp.listRemoteProcess"
},
"program": "./out/src/coreclr-debug/proxy.js",
Copy link
Member

Choose a reason for hiding this comment

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

nor this

package.json Outdated
"program": "./.debugger/vsdbg-ui"
},
"linux": {
"program": "./.debugger/vsdbg-ui"
Copy link
Member

Choose a reason for hiding this comment

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

Or this

@gep13
Copy link

gep13 commented Aug 4, 2017

@mholo65 was there something in particular that you thought was "wrong" with this PR? The changes look good to me. 👍

Copy link

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM

@bjorkstromm
Copy link
Contributor Author

@gep13 well if we could figure out why we have to change language mode from Cake to C# before this extension wants to send any requests to omnisharp.exe, that would be great 😉

omnisharp.exe is started when a.cake file is opened, but no requests are sent. Current fix is to switch language mode

@DustinCampbell
Copy link
Member

well if we could figure out why we have to change language mode from Cake to C# before this extension wants to send any requests to omnisharp.exe, that would be great 😉

I posted how that happens in the #cake channel on Slack. The file types that trigger "csharp" as the language is defined in VS Code itself right here. I'll submit a change to add "*.cake" to this list on your behalf today.

@gep13
Copy link

gep13 commented Aug 4, 2017

@DustinCampbell said...
The file types that trigger "csharp" as the language is defined in VS Code itself right here. I'll submit a change to add "*.cake" to this list on your behalf today.

Yes, I saw that in the Slack channel, and that is why I raised this issue #1684, I was trying to get clarity on whether the syntax highlighting that we provide for .cake files would be affected by making that change.

Do you know if this will be the case?

@DustinCampbell
Copy link
Member

Yes, the syntax highlighting for .cake files would be affected. What differences does Cake add to C# syntax highlighting? Note that the official C# support for cake highlighting is here: https://github.com/dotnet/csharp-tmLanguage.

@DustinCampbell
Copy link
Member

I think this is generally a problem of trying to spread Cake support across two extensions. If you're trying to have a Cake Support extension provide syntax highlighting and the C# extension, I'm less comfortable with taking this change.

@DustinCampbell
Copy link
Member

Cake is really just C# script with some semantic differences, correct? If that's the case, I would proposal that the Cake Support extension doesn't provide it's own syntax highlighting, but takes advantage of C#'s. Then, the Cake Support extension would be modified to operate on the "csharp" language but only when the extension is ".cake". Thoughts?

@DustinCampbell
Copy link
Member

I'm going to go ahead and move this conversation over to #1684.

@DustinCampbell
Copy link
Member

This looks good. Sorry for the delay!

@DustinCampbell DustinCampbell merged commit 1af9733 into dotnet:master Oct 5, 2017
@bjorkstromm bjorkstromm deleted the feature/cake branch October 5, 2017 19:12
@bjorkstromm bjorkstromm restored the feature/cake branch October 5, 2017 19:13
@gep13
Copy link

gep13 commented Oct 5, 2017

WOO HOO! Thanks for merging this!

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.

4 participants