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 activation events for *.csproj and *.sln #1592

Merged
merged 3 commits into from
Jun 29, 2017

Conversation

DustinCampbell
Copy link
Member

Should fix #1375 and improve #1150.

@DustinCampbell
Copy link
Member Author

cc @eamodio

@DustinCampbell
Copy link
Member Author

Is this actually a problem for *.csx? Note that the extension already activates on *.csx files when they're opened because that's how the "csharp" language is defined in VS Code (link).

Note I'm not adding *.cs because that seems a bit too invasive. Could you explain how *.csx would be different.

@DustinCampbell
Copy link
Member Author

Note I'm not adding *.cs because that seems a bit too invasive.

I'm totally willing to discuss this bit. That's my first gut feeling, but I'm not 100% certain it's something I should be concerned about. I definitely wouldn't want to add **/*.cs because a random C# file somewhere in a subfolder could cause OmniSharp to launch. However, if there's a .cs file in the root?

@filipw
Copy link
Contributor

filipw commented Jun 26, 2017

I think csx case would be different than cs, because csx is a de facto project file - either as a fully self contained app or as an entry point that may contain references to other csx files (so other compilation items) as well as assembly references.

A standalone cs file could indeed have no meaning if it doesn't have a project file, as it would not know how to compile itself (plus, Omnisharp wouldn't even know how to treat such loose cs files). OTOH, a loose csx file (or a bunch of them) is always a valid start point for the server, and we can always provide proper language services.

It would be nice to open a folder with some csx scripts in there and have Omnisharp light up immediately so that:

  • you avoid the little delay to start the server when a csx is actually open
  • you can immediately do things like i.e. go to definition

@DustinCampbell
Copy link
Member Author

you can immediately do things like i.e. go to definition

How does this help that? After all, you have to have a file open to go to definition, no?

@filipw
Copy link
Contributor

filipw commented Jun 26, 2017

sorry, I meant "go to symbol"

@DustinCampbell
Copy link
Member Author

I suppose that's true. OK. I'll add *.csx as well. I still think *.cs is too much though. Do you disagree?

@filipw
Copy link
Contributor

filipw commented Jun 26, 2017

no, I think you right about that 👍
thanks!

@@ -340,7 +340,9 @@
"onCommand:csharp.downloadDebugger",
"onCommand:csharp.listProcess",
"onCommand:csharp.listRemoteProcess",
"workspaceContains:project.json"
"workspaceContains:project.json",
"workspaceContains:*.csproj",
Copy link
Contributor

@eamodio eamodio Jun 26, 2017

Choose a reason for hiding this comment

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

@DustinCampbell
I'm not 100% sure, but should those be workspaceContains:**/*.csproj so that it could be in any folder? It may already be handled though

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally didn't do that but I'm open to discussing it. I feared that this might be a bit invasive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah -- personally I would definitely want that. We typically open vscode to the root of the repository, and it doesn't have any projects or anything in it -- they are all nested under sub-folders. What are the cases you are thinking about where you wouldn't want that behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of scenarios like the one described by #1328.

@DustinCampbell
Copy link
Member Author

OK. I've added patterns for *.csx and globs to search subfolders. I'm planning to ship 1.11 shortly and would like to hold this change for 1.12 to let folks play with it to ensure it feels good. Does that work for everybody?

@filipw
Copy link
Contributor

filipw commented Jun 27, 2017

sounds like a plan 👍

@DustinCampbell
Copy link
Member Author

OK. I'm going to go ahead and merge this for 1.12.

@DustinCampbell DustinCampbell merged commit bebeaf4 into dotnet:master Jun 29, 2017
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.

'Go to Symbol in Workspace' doesn't work unless there is an open C# file
4 participants