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.

Sign up for GitHub

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

Jump to bottom

Get root namespace from the project #30998

Merged
merged 3 commits into from
Dec 5, 2018
Merged

Get root namespace from the project #30998

merged 3 commits into from
Dec 5, 2018

Conversation

genlu
Copy link
Member

@genlu genlu commented Nov 6, 2018 •
edited
Loading

This is the implementation for the API added in #31254.

Also this include the change that (hopefully) clarified the "default namespace vs root namespace for VB" issue. Right now VB doesn't have the concept of "default namespace". In this change, we conjure one in workspace, and assign the value of the project's root namespace to it for now. Individual IDE features can choose to use it for their own purpose. This would enable us to introduce "default namespace" for VB project in the future. One possibility is through a new <defaultnamespace> msbuild property which by default set to the value of <rootnamespace>, but user can choose to set it to something else when <rootnamespace> is left empty, i.e. root namespace is Global and IDE can enforce namespace declaration in the project based on the value of <defaultnamespace>

FYI @jasonmalinowski @davkean

@genlu genlu requested a review from a team as a code owner November 6, 2018 22:14
@genlu genlu closed this Nov 7, 2018
@genlu genlu reopened this Nov 7, 2018
@jinujoseph jinujoseph added this to the 16.0.P2 milestone Nov 8, 2018
jasonmalinowski
jasonmalinowski reviewed Nov 9, 2018
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Generally OK, but a few of @CyrusNajmabadi's comments do raise good points.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContext.cs Outdated Show resolved Hide resolved
src/VisualStudio/Core/Impl/ProjectSystem/CPS/AdditionalPropertyNames.cs Outdated Show resolved Hide resolved
src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs Show resolved Hide resolved
src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs Outdated Show resolved Hide resolved
src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs Outdated Show resolved Hide resolved
src/Workspaces/Core/Portable/Workspace/Workspace.cs Outdated Show resolved Hide resolved
@genlu
Copy link
Member Author

genlu commented Nov 13, 2018

@jasonmalinowski Could you please take another look?

jasonmalinowski
jasonmalinowski previously requested changes Nov 13, 2018
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Still seems like there's one bug where we should be initializing this to empty string for all C# projects. ASP.NET backing projects really need to have that be null, I suspect, because we don't want to act on it.

@genlu
Copy link
Member Author

genlu commented Nov 13, 2018 •
edited
Loading

@jasonmalinowski I have addressed your comments, please take another look. Also could you please elaborate on your comment re: ASP.NET?

jasonmalinowski
jasonmalinowski reviewed Nov 14, 2018
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Removing block, but I still really would like to see a plan for what VB/F# looks like, even if we don't implement it.

@jasonmalinowski jasonmalinowski dismissed their stale review November 14, 2018 00:30

Close, at this point all I think we need to do is resolve the conversation for other languages here.

@genlu
Copy link
Member Author

genlu commented Nov 15, 2018

@jasonmalinowski So as mentioned offline, I have made the change to expose root namespace directly, instead of default namespace for C# projects.

One thing that's not working properly is when changing the rootnamespace property value for legacy project through project property (project -> <project name> properties... -> Default namespace (for C#)/Root namespace (for VB)), there's no event notifying us about the change.

This commit (10bc122) is an attempt to hack around this issue for legacy VB project, but I ended up reverting it because (1) I really don't like it and (2) the feature I'm working on doesn't even need it from VB project. So I will just file a bug for this and figure out if I can listen to the property change from VS.

@jasonmalinowski jasonmalinowski changed the base branch from dev16.0-preview2 to master November 16, 2018 19:43
@genlu genlu mentioned this pull request Nov 19, 2018
Merged
@genlu genlu changed the title Callback API for project system to pass in arbitrary properties Get projectroot namespace from project Nov 19, 2018
@genlu genlu changed the title Get projectroot namespace from project Get root namespace from the project Nov 19, 2018
@genlu
Copy link
Member Author

genlu commented Nov 19, 2018 •
edited
Loading

Separated the API change to another PR: #31254

Squashed all commits and fixed sync namespace refactoring to query rootnamespace instead.

Also, according to @tmeschter, there's no event we can listen to for the change to root/default namespace field in project property page in VS (see screen below), which means for legacy project system, the value will be outdated once user changed it through UI. Do you guys know if there's any way to address this issue? @CyrusNajmabadi @davkean ?

imgo

@genlu
Make default namespace available for VB in workspace and implement Se…
fa654f0 
…tProperty for CPS project.

Right now VB doesn't have the concept of "default namespace". But we conjure one in workspace by assigning the value of the project's root namespace to it. So various feature can choose to use it for their own purpose. In the future, we might consider officially exposing "default namespace" for VB project (e.g. through a <defaultnamespace> msbuild property)
@genlu
Copy link
Member Author

genlu commented Nov 20, 2018 •
edited
Loading

@jasonmalinowski OK, did one more iteration per our offline discussion and updated the PR description. Please let me know if you have any additional concern re: default namespace in VB.

@genlu
Copy link
Member Author

genlu commented Nov 21, 2018

@jinujoseph @vatsalyaagrawal for Preview2 approval.

@genlu genlu mentioned this pull request Nov 29, 2018
Open
15 tasks
@genlu
Copy link
Member Author

genlu commented Dec 3, 2018

@jasonmalinowski Could you please take another look?

jasonmalinowski
jasonmalinowski approved these changes Dec 4, 2018
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

A few questions, but looks good. I'm excited that you're able to pull the property from the IVsHierarchy, did you test to see if they do raise a hierarchy event when it changes? If so, then that'll mean we solve that issue too.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProject.cs
@@ -278,6 +281,24 @@ internal bool HasAllInformation
w => w.OnHasAllInformationChanged(Id, value));
}

/// <summary>
/// The default namespace of the project.
/// In C#, this is defined as the value of "rootnamespace" msbuild property. Right now VB doesn't
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the discussion to a <remarks> block? I always think of summary as "the thing in quick info" and that's a bit verbose.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

src/VisualStudio/Core/Def/Implementation/ProjectSystem/CPS/IWorkspaceProjectContext.cs Outdated
@@ -23,6 +23,9 @@ internal interface IWorkspaceProjectContext : IDisposable
// Options.
void SetOptions(string commandLineForOptions);

// Other project properties.
void SetProperty(string name, string value);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already in master? Why are we seeing the diff here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Went away after sync

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Legacy/AbstractLegacyProject.cs
/// </remarks>>
private static string GetDefaultNamespace(IVsHierarchy hierarchy, string language)
private static string GetRootNamespacePropertyValue(IVsHierarchy hierarchy)
Copy link
Member

Choose a reason for hiding this comment

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

So if we're able to fetch this from the hierarchy, will they raise a hierarchy event when the property changes? If so, that'd be fantastic.

Copy link
Member Author

@genlu genlu Dec 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, that's not the case.But I'm not too concerned about this scenario (change it through "project properties" UI), because (1) it's not very common and (2) we are moving legacy to IWorkspaceProjectContext

@genlu genlu merged commit dd3b0e3 into dotnet:master Dec 5, 2018
@genlu genlu deleted the projectsystem-properties branch December 5, 2018 19:44
@felixhirt felixhirt mentioned this pull request Feb 28, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-IDE
Projects
None yet
Milestone
16.0.P2
Development

Successfully merging this pull request may close these issues.
4 participants
@genlu @jasonmalinowski @CyrusNajmabadi @jinujoseph