-
Notifications
You must be signed in to change notification settings - Fork 688
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
Refactoring ErrorList into NuGet.VisualStudio.Common #2340
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.
LGTM.
A few minor questions.
@@ -222,12 +222,14 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NuGet.Shared.Tests", "test\ | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NuGet.MSSigning.Extensions.FuncTest", "test\NuGet.Clients.FuncTests\NuGet.MSSigning.Extensions.FuncTest\NuGet.MSSigning.Extensions.FuncTest.csproj", "{BA7F681E-FDC7-42B6-8BF1-74DA6421BA1A}" | |||
EndProject | |||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NuGet.MSSigning.Extensions.Test", "test\NuGet.Clients.Tests\NuGet.MSSigning.Extensions.Test\NuGet.MSSigning.Extensions.Test.csproj", "{2A604A43-C72A-4A03-B68B-66AF4B6F430E}" |
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.
Why is the project Guid changing here?
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.
Also updating the guid for test projects based off of this comment.
@@ -106,10 +106,6 @@ | |||
<Project>{538adefd-2170-40a9-a2c5-ec8369cfe490}</Project> | |||
<Name>NuGet.PackageManagement.UI</Name> | |||
</ProjectReference> | |||
<ProjectReference Include="..\NuGet.SolutionRestoreManager\NuGet.SolutionRestoreManager.csproj"> |
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.
👏
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" /> |
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.
Do we need these?
@@ -23,6 +23,7 @@ | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.Console" Path="|NuGet.Console|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.VisualStudio.Implementation" Path="|NuGet.VisualStudio.Implementation|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.SolutionRestoreManager" Path="|NuGet.SolutionRestoreManager|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.VisualStudio.Common" Path="|NuGet.VisualStudio.Common|" /> |
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.
why add here? Should we continue to manage dev14?
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.
larger question. Unless we decide to remove this, I think this is okay.
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.
We should remove everything dev 14 related from the codebase.
@@ -23,6 +23,7 @@ | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.Console" Path="|NuGet.Console|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.VisualStudio.Implementation" Path="|NuGet.VisualStudio.Implementation|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.SolutionRestoreManager" Path="|NuGet.SolutionRestoreManager|" /> | |||
<Asset Type="Microsoft.VisualStudio.MefComponent" d:Source="Project" d:ProjectName="NuGet.VisualStudio.Common" Path="|NuGet.VisualStudio.Common|" /> |
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.
Ahh we had to add it to MEFComponent list. But will it have any adverse effect on VS initialization or load for an additional MEF? As I've mentioned multiple times, this change doesn't make any difference on ground level since NuGet.SolutionRestoreManager
is always the first NuGet package being loaded inside VS.
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.
I know. I will run RPS before checking this in.
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.
RPS result for this branch is here. I see no new regression when compared to the last dev run.
@@ -260,18 +261,18 @@ private IVsPathContext2 GetSolutionPathContext(string packagesFolderPath) | |||
if (string.IsNullOrEmpty(projectNameFromMSBuildPath)) | |||
{ | |||
projectJsonPath = Path.Combine(msbuildProjectFile.DirectoryName, | |||
Common.ProjectJsonPathUtilities.ProjectConfigFileName); | |||
ProjectJsonPathUtilities.ProjectConfigFileName); |
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.
why this file is being changed?
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.
namespace changes caused compiler errors between NuGet.Common
and NuGet.VisualStudio.Common
.
a814386
to
75c080f
Compare
75c080f
to
a71a96c
Compare
@mishra14 Did this cause RPS problems? |
@nkolev92 not sure yet. I was just rebasing from dev and github decided to close this 😆 i will reopen it. |
@jainaashish 🔔 |
Follow up to #2313
As requested by @nkolev92, I have refactored the code to move
ErrorListTableEntry.cs
andErrorListTableDataSource.cs
fromNuGet.SolutionRestoreManager
toNuGet.VisualStudio.Common
and removed the dependency fromNuGet.Tools
toNuGet.SolutionRestoreManager
Also updating the guid for test projects based off of this comment.