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

Added support for "sync namespace" refactoring #1563

Merged
merged 23 commits into from
Jul 31, 2019

Conversation

filipw
Copy link
Member

@filipw filipw commented Jul 19, 2019

Fixes #1475

Demo (first from within a folder, and then from the root of the project):

tjh31xd8tC

We have actually PR-ed into Roslyn a change that opened up the necessary APIs to public dotnet/roslyn#35486 to make it work.
This PR introduces a bump to Roslyn 3.3.0-beta1-19365-07 to have access to them - this is a build for the dev16.3 release train.

We also now track "folders" of the documents added to the workspace (the refactoring works off that) - which is basically a relative path.
Default namespace is, by default, the assembly name, unless overwritten by <RootNamespace> MsBuild property. A change in MsBuild project file is respected by us in real time (no need to restart OmniSharp), by updating the workspace accordingly. Unfortunately there is still a bug in Roslyn where the default namespace change doesn't propagate correctly via TryApplyChanges on the solution, so we have a small reflection workaround for that. I will PR the necessary change into Roslyn and once it's available we can remove the workaround.

cc @savpek

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Only one question about the behavior otherwise it looks good to me.

}
else
{
base.ApplyProjectChanges(projectChanges);
Copy link
Member

Choose a reason for hiding this comment

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

Should we call the base ApplyProjectChanges all the time or only if the namespace didn't change? Would this possibly miss some changes if they were batched together with the namespace change?

Copy link
Member Author

@filipw filipw Jul 19, 2019

Choose a reason for hiding this comment

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

thanks @david-driscoll this is a great catch - changed accordingly 👍

Copy link
Contributor

@savpek savpek left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@filipw filipw mentioned this pull request Jul 31, 2019
@akshita31 akshita31 merged commit ba7909c into OmniSharp:master Jul 31, 2019
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.

Make new SyncNamespace refactoring work with omnisharp-roslyn
4 participants