Address PR comments
This commit is contained in:
		
					parent
					
						
							
								037da3fc01
							
						
					
				
			
			
				commit
				
					
						3821d39d6c
					
				
			
		
					 5 changed files with 88 additions and 31 deletions
				
			
		|  | @ -955,6 +955,21 @@ namespace Microsoft.DotNet.Internal.ProjectModel | ||||||
|             return (rawProject.SelectToken(jpathToNewFormatObject) != null); |             return (rawProject.SelectToken(jpathToNewFormatObject) != null); | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|  |         private static void ConvertFromDeprecatedFormat( | ||||||
|  |             JObject rawProject, | ||||||
|  |             string jpathToObject, | ||||||
|  |             string deprecatedKey, | ||||||
|  |             string newKey | ||||||
|  |             ) | ||||||
|  |         { | ||||||
|  |             var deprecatedValue = rawProject.Value<JToken>(deprecatedKey); | ||||||
|  |             if (deprecatedValue != null) | ||||||
|  |             { | ||||||
|  |                 var objectNode = GetOrCreateObjectHierarchy(rawProject, jpathToObject); | ||||||
|  |                 objectNode[newKey] = deprecatedValue.DeepClone(); | ||||||
|  |             } | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|         private static JObject GetOrCreateObjectHierarchy(JObject rawProject, string jpath) |         private static JObject GetOrCreateObjectHierarchy(JObject rawProject, string jpath) | ||||||
|         { |         { | ||||||
|             var currentObject = rawProject as JObject; |             var currentObject = rawProject as JObject; | ||||||
|  | @ -973,21 +988,6 @@ namespace Microsoft.DotNet.Internal.ProjectModel | ||||||
| 
 | 
 | ||||||
|             return currentObject; |             return currentObject; | ||||||
|         } |         } | ||||||
|          |  | ||||||
|         private static void ConvertFromDeprecatedFormat( |  | ||||||
|             JObject rawProject, |  | ||||||
|             string jpathToObject, |  | ||||||
|             string deprecatedKey, |  | ||||||
|             string newKey |  | ||||||
|             ) |  | ||||||
|         { |  | ||||||
|             var deprecatedValue = rawProject.Value<JToken>(deprecatedKey); |  | ||||||
|             if (deprecatedValue != null) |  | ||||||
|             { |  | ||||||
|                 var objectNode = GetOrCreateObjectHierarchy(rawProject, jpathToObject); |  | ||||||
|                 objectNode[newKey] = deprecatedValue.DeepClone(); |  | ||||||
|             } |  | ||||||
|         } |  | ||||||
| 
 | 
 | ||||||
|         private static bool TryGetStringEnumerable(JToken token, out IEnumerable<string> result) |         private static bool TryGetStringEnumerable(JToken token, out IEnumerable<string> result) | ||||||
|         { |         { | ||||||
|  |  | ||||||
|  | @ -42,10 +42,11 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|             IEnumerable<ProjectDependency> projectDependencies = null; |             IEnumerable<ProjectDependency> projectDependencies = null; | ||||||
|             var projectMigrationReports = new List<ProjectMigrationReport>(); |             var projectMigrationReports = new List<ProjectMigrationReport>(); | ||||||
| 
 | 
 | ||||||
|  |             List<string> warnings = null; | ||||||
|             try |             try | ||||||
|             { |             { | ||||||
|                 // Verify up front so we can prefer these errors over an unresolved project dependency |                 // Verify up front so we can prefer these errors over an unresolved project dependency | ||||||
|                 VerifyInputs(rootInputs, rootSettings); |                 VerifyInputs(rootInputs, rootSettings, out warnings); | ||||||
| 
 | 
 | ||||||
|                 projectMigrationReports.Add(MigrateProject(rootSettings)); |                 projectMigrationReports.Add(MigrateProject(rootSettings)); | ||||||
|                  |                  | ||||||
|  | @ -68,7 +69,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|                             rootSettings.ProjectDirectory, |                             rootSettings.ProjectDirectory, | ||||||
|                             rootInputs?.DefaultProjectContext?.GetProjectName(), |                             rootInputs?.DefaultProjectContext?.GetProjectName(), | ||||||
|                             new List<MigrationError> {e.Error}, |                             new List<MigrationError> {e.Error}, | ||||||
|                             null) |                             warnings) | ||||||
|                     }); |                     }); | ||||||
|             } |             } | ||||||
| 
 | 
 | ||||||
|  | @ -144,6 +145,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|             var projectName = migrationRuleInputs.DefaultProjectContext.GetProjectName(); |             var projectName = migrationRuleInputs.DefaultProjectContext.GetProjectName(); | ||||||
|             var outputProject = Path.Combine(migrationSettings.OutputDirectory, projectName + ".csproj"); |             var outputProject = Path.Combine(migrationSettings.OutputDirectory, projectName + ".csproj"); | ||||||
| 
 | 
 | ||||||
|  |             List<string> warnings = null; | ||||||
|             try |             try | ||||||
|             { |             { | ||||||
|                 if (File.Exists(outputProject)) |                 if (File.Exists(outputProject)) | ||||||
|  | @ -166,7 +168,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|                     } |                     } | ||||||
|                 } |                 } | ||||||
| 
 | 
 | ||||||
|                 VerifyInputs(migrationRuleInputs, migrationSettings); |                 VerifyInputs(migrationRuleInputs, migrationSettings, out warnings); | ||||||
| 
 | 
 | ||||||
|                 SetupOutputDirectory(migrationSettings.ProjectDirectory, migrationSettings.OutputDirectory); |                 SetupOutputDirectory(migrationSettings.ProjectDirectory, migrationSettings.OutputDirectory); | ||||||
| 
 | 
 | ||||||
|  | @ -179,7 +181,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|                     exc.Error |                     exc.Error | ||||||
|                 }; |                 }; | ||||||
| 
 | 
 | ||||||
|                 return new ProjectMigrationReport(migrationSettings.ProjectDirectory, projectName, error, null); |                 return new ProjectMigrationReport(migrationSettings.ProjectDirectory, projectName, error, warnings); | ||||||
|             } |             } | ||||||
| 
 | 
 | ||||||
|             List<string> csprojDependencies = null; |             List<string> csprojDependencies = null; | ||||||
|  | @ -208,7 +210,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|                 projectName, |                 projectName, | ||||||
|                 outputProject, |                 outputProject, | ||||||
|                 null, |                 null, | ||||||
|                 null, |                 warnings, | ||||||
|                 csprojDependencies); |                 csprojDependencies); | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|  | @ -235,13 +237,22 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|             return new MigrationRuleInputs(projectContexts, templateMSBuildProject, itemGroup, propertyGroup, xproj); |             return new MigrationRuleInputs(projectContexts, templateMSBuildProject, itemGroup, propertyGroup, xproj); | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|         private void VerifyInputs(MigrationRuleInputs migrationRuleInputs, MigrationSettings migrationSettings) |         private void VerifyInputs( | ||||||
|  |             MigrationRuleInputs migrationRuleInputs, | ||||||
|  |             MigrationSettings migrationSettings, | ||||||
|  |             out List<string> warningMessages | ||||||
|  |             ) | ||||||
|         { |         { | ||||||
|             VerifyProject(migrationRuleInputs.ProjectContexts, migrationSettings.ProjectDirectory); |             VerifyProject(migrationRuleInputs.ProjectContexts, migrationSettings.ProjectDirectory, out warningMessages); | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|         private void VerifyProject(IEnumerable<ProjectContext> projectContexts, string projectDirectory) |         private void VerifyProject( | ||||||
|  |             IEnumerable<ProjectContext> projectContexts, | ||||||
|  |             string projectDirectory, | ||||||
|  |             out List<string> warningMessages) | ||||||
|         { |         { | ||||||
|  |             warningMessages = null; | ||||||
|  | 
 | ||||||
|             if (!projectContexts.Any()) |             if (!projectContexts.Any()) | ||||||
|             { |             { | ||||||
|                 MigrationErrorCodes.MIGRATE1013(String.Format(LocalizableStrings.MIGRATE1013Arg, projectDirectory)).Throw(); |                 MigrationErrorCodes.MIGRATE1013(String.Format(LocalizableStrings.MIGRATE1013Arg, projectDirectory)).Throw(); | ||||||
|  | @ -255,11 +266,14 @@ namespace Microsoft.DotNet.ProjectJsonMigration | ||||||
|                 var warnings = diagnostics.Where(d => d.Severity == DiagnosticMessageSeverity.Warning); |                 var warnings = diagnostics.Where(d => d.Severity == DiagnosticMessageSeverity.Warning); | ||||||
|                 if (warnings.Any()) |                 if (warnings.Any()) | ||||||
|                 { |                 { | ||||||
|                     var deprecatedProjectJsonWarnings = string.Join( |                     var migrationError = MigrationErrorCodes.MIGRATE1011(String.Format( | ||||||
|  |                         "{0}{1}{2}", | ||||||
|  |                         projectDirectory, | ||||||
|                         Environment.NewLine, |                         Environment.NewLine, | ||||||
|                         diagnostics.Select(d => FormatDiagnosticMessage(d))); |                         string.Join(Environment.NewLine, diagnostics.Select(d => FormatDiagnosticMessage(d))))); | ||||||
|                     var warningMessage = $"{projectDirectory}{Environment.NewLine}{deprecatedProjectJsonWarnings}"; | 
 | ||||||
|                     Reporter.Output.WriteLine(warningMessage.Yellow()); |                     warningMessages = new List<string>(); | ||||||
|  |                     warningMessages.Add(migrationError.GetFormattedErrorMessage()); | ||||||
|                 } |                 } | ||||||
| 
 | 
 | ||||||
|                 var errors = diagnostics.Where(d => d.Severity == DiagnosticMessageSeverity.Error); |                 var errors = diagnostics.Where(d => d.Severity == DiagnosticMessageSeverity.Error); | ||||||
|  |  | ||||||
|  | @ -262,6 +262,8 @@ namespace Microsoft.DotNet.Tools.Migrate | ||||||
|             { |             { | ||||||
|                 var errorContent = GetProjectReportErrorContent(projectMigrationReport, colored: true); |                 var errorContent = GetProjectReportErrorContent(projectMigrationReport, colored: true); | ||||||
|                 var successContent = GetProjectReportSuccessContent(projectMigrationReport, colored: true); |                 var successContent = GetProjectReportSuccessContent(projectMigrationReport, colored: true); | ||||||
|  |                 var warningContent = GetProjectReportWarningContent(projectMigrationReport, colored: true); | ||||||
|  |                 Reporter.Output.WriteLine(warningContent); | ||||||
|                 if (!string.IsNullOrEmpty(errorContent)) |                 if (!string.IsNullOrEmpty(errorContent)) | ||||||
|                 { |                 { | ||||||
|                     Reporter.Error.WriteLine(errorContent); |                     Reporter.Error.WriteLine(errorContent); | ||||||
|  | @ -290,6 +292,8 @@ namespace Microsoft.DotNet.Tools.Migrate | ||||||
|             { |             { | ||||||
|                 var errorContent = GetProjectReportErrorContent(projectMigrationReport, colored: colored); |                 var errorContent = GetProjectReportErrorContent(projectMigrationReport, colored: colored); | ||||||
|                 var successContent = GetProjectReportSuccessContent(projectMigrationReport, colored: colored); |                 var successContent = GetProjectReportSuccessContent(projectMigrationReport, colored: colored); | ||||||
|  |                 var warningContent = GetProjectReportWarningContent(projectMigrationReport, colored: colored); | ||||||
|  |                 sb.AppendLine(warningContent); | ||||||
|                 if (!string.IsNullOrEmpty(errorContent)) |                 if (!string.IsNullOrEmpty(errorContent)) | ||||||
|                 { |                 { | ||||||
|                     sb.AppendLine(errorContent); |                     sb.AppendLine(errorContent); | ||||||
|  | @ -331,6 +335,19 @@ namespace Microsoft.DotNet.Tools.Migrate | ||||||
|                 projectMigrationReport.ProjectDirectory)); |                 projectMigrationReport.ProjectDirectory)); | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|  |         private string GetProjectReportWarningContent(ProjectMigrationReport projectMigrationReport, bool colored) | ||||||
|  |         { | ||||||
|  |             StringBuilder sb = new StringBuilder(); | ||||||
|  |             Func<string, string> YellowIfColored = (str) => colored ? str.Yellow() : str; | ||||||
|  | 
 | ||||||
|  |             foreach (var warning in projectMigrationReport.Warnings) | ||||||
|  |             { | ||||||
|  |                 sb.AppendLine(YellowIfColored(warning)); | ||||||
|  |             } | ||||||
|  | 
 | ||||||
|  |             return sb.ToString(); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|         private string GetProjectReportErrorContent(ProjectMigrationReport projectMigrationReport, bool colored) |         private string GetProjectReportErrorContent(ProjectMigrationReport projectMigrationReport, bool colored) | ||||||
|         { |         { | ||||||
|             StringBuilder sb = new StringBuilder(); |             StringBuilder sb = new StringBuilder(); | ||||||
|  |  | ||||||
|  | @ -39,6 +39,32 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests | ||||||
|             } |             } | ||||||
|         } |         } | ||||||
| 
 | 
 | ||||||
|  |         [Fact] | ||||||
|  |         public void ItHasWarningWhenMigratingADeprecatedProjectJson() | ||||||
|  |         { | ||||||
|  |             var testProjectDirectory = TestAssets | ||||||
|  |                .GetProjectJson(TestAssetKinds.NonRestoredTestProjects, "PJAppWithDeprecatedCompileOptions") | ||||||
|  |                .CreateInstance() | ||||||
|  |                .WithSourceFiles() | ||||||
|  |                .Root | ||||||
|  |                .GetDirectory("project") | ||||||
|  |                .FullName; | ||||||
|  | 
 | ||||||
|  |             var mockProj = ProjectRootElement.Create(); | ||||||
|  |             var testSettings = MigrationSettings.CreateMigrationSettingsTestHook( | ||||||
|  |                 testProjectDirectory, | ||||||
|  |                 testProjectDirectory, | ||||||
|  |                 mockProj); | ||||||
|  | 
 | ||||||
|  |             var projectMigrator = new ProjectMigrator(new FakeEmptyMigrationRule()); | ||||||
|  |             var report = projectMigrator.Migrate(testSettings); | ||||||
|  | 
 | ||||||
|  |             var projectReport = report.ProjectMigrationReports.First(); | ||||||
|  |             var warningMessage = projectReport.Warnings.First(); | ||||||
|  |             warningMessage.Should().Contain("MIGRATE1011::Deprecated Project:"); | ||||||
|  |             warningMessage.Should().Contain("The 'compile' option is deprecated. Use 'compile' in 'buildOptions' instead. (line: 3, file:"); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|         [Fact] |         [Fact] | ||||||
|         public void ItHasErrorWhenMigratingADeprecatedNamedResourceOptionProjectJson() |         public void ItHasErrorWhenMigratingADeprecatedNamedResourceOptionProjectJson() | ||||||
|         { |         { | ||||||
|  |  | ||||||
|  | @ -72,12 +72,12 @@ namespace Microsoft.DotNet.Migration.Tests | ||||||
| 
 | 
 | ||||||
|             new DotnetCommand() |             new DotnetCommand() | ||||||
|                  .WithWorkingDirectory(projectDirectory) |                  .WithWorkingDirectory(projectDirectory) | ||||||
|                  .Execute("build") |                  .Execute("build -c Debug") | ||||||
|                  .Should().Pass(); |                  .Should().Pass(); | ||||||
| 
 | 
 | ||||||
|             new DotnetCommand() |             new DotnetCommand() | ||||||
|                  .WithWorkingDirectory(projectDirectory) |                  .WithWorkingDirectory(projectDirectory) | ||||||
|                  .Execute("pack") |                  .Execute("pack -c Debug") | ||||||
|                  .Should().Pass(); |                  .Should().Pass(); | ||||||
| 
 | 
 | ||||||
|             var outputDir = projectDirectory.GetDirectory("bin", "Debug"); |             var outputDir = projectDirectory.GetDirectory("bin", "Debug"); | ||||||
|  | @ -212,12 +212,12 @@ namespace Microsoft.DotNet.Migration.Tests | ||||||
| 
 | 
 | ||||||
|             new DotnetCommand() |             new DotnetCommand() | ||||||
|                  .WithWorkingDirectory(projectDirectory) |                  .WithWorkingDirectory(projectDirectory) | ||||||
|                  .Execute("build") |                  .Execute("build -c Debug") | ||||||
|                  .Should().Pass(); |                  .Should().Pass(); | ||||||
| 
 | 
 | ||||||
|             new DotnetCommand() |             new DotnetCommand() | ||||||
|                  .WithWorkingDirectory(projectDirectory) |                  .WithWorkingDirectory(projectDirectory) | ||||||
|                  .Execute("publish") |                  .Execute("publish -c Debug") | ||||||
|                  .Should().Pass(); |                  .Should().Pass(); | ||||||
| 
 | 
 | ||||||
|             var outputDir = projectDirectory.GetDirectory("bin", "Debug", "netcoreapp1.0"); |             var outputDir = projectDirectory.GetDirectory("bin", "Debug", "netcoreapp1.0"); | ||||||
|  |  | ||||||
		Reference in a new issue
	
	 Justin Goshi
				Justin Goshi