-
Notifications
You must be signed in to change notification settings - Fork 420
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
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Fixes #1475
Demo (first from within a folder, and then from the root of the project):
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 thedev16.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 viaTryApplyChanges
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