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

Watch .csx files in order to add/remove script projects to workspace. #1056

Merged
merged 3 commits into from
Dec 13, 2017

Conversation

bjorkstromm
Copy link
Member

Builds upon PR #1053. (Merge that first, and then rebase this...)

Add created .csx files to workspace and remove deleted .csx files from workspace.

ping @filipw

if (changeType == FileChangeType.Unspecified && File.Exists(filePath) ||
changeType == FileChangeType.Create)
{
AddToWorkspace(filePath);
Copy link
Member

Choose a reason for hiding this comment

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

Here we'll want to run TryGetCompilationDependencies again, because that would resolve #r nuget: and #load nuget: if these are used inside of the file. However, there is no API at the moment to do it other than for a current folder so this needs to go on a TODO list for later. I plan to adjust some APIs to facilitate that soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Dependencies are not added automatically by the NuGet Resolvers (NuGetSourceReferenceResolver and NuGetMetadataReferenceResolver)?

Copy link
Member

Choose a reason for hiding this comment

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

That is the long term goal, but at the moment they can't due to a current limitation in Roslyn which prevents a reference from resolving into more than a single MetadataReference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, in that case, will it add any value to run TryGetCompilationDependencies again here? Since a recently added file will most of the time be blank... Unless, it’s a rename, move or copy from within VSCode?

Copy link
Member

Choose a reason for hiding this comment

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

I think for now let's just leave it as is - and when we upgrade to "self contained" resolvers the problem will fix itself. If that doesn't come soon enough, we'll do something smarter here - what you have in this PR is a big step forward anyway 👍

Copy link
Member

Choose a reason for hiding this comment

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

Not to mention for the primary scenario we want to support - CSI - it will already work as expected.

@filipw filipw added the csx label Dec 12, 2017
Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

:shipit:

@bjorkstromm bjorkstromm force-pushed the feature/watch-csx-files branch from 06340f2 to 3915f1b Compare December 12, 2017 20:40
@bjorkstromm
Copy link
Member Author

@DustinCampbell changelog updated.

@filipw
Copy link
Member

filipw commented Dec 13, 2017

@mholo65 there is a changelog conflict that needs resolving now 😄

@filipw
Copy link
Member

filipw commented Dec 13, 2017

thanks!

@bjorkstromm
Copy link
Member Author

@filipw resolved conflict using Github GUI.

@david-driscoll david-driscoll merged commit 5083515 into OmniSharp:master Dec 13, 2017
@bjorkstromm bjorkstromm deleted the feature/watch-csx-files branch December 14, 2017 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants