Skip to content

Fix breaking change in Add-Type #5306

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

Merged
daxian-dbw merged 1 commit into from
Nov 3, 2017
Merged

Fix breaking change in Add-Type #5306

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 56 additions & 9 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,17 +760,10 @@ internal string GetUsingSet(Language language)
/// <summary>
/// Perform common error checks.
/// Populate source code.
/// We only keep the code for backward compatibility.
/// </summary>
protected override void EndProcessing()
{
// Prevent code compilation in ConstrainedLanguage mode
if (SessionState.LanguageMode == PSLanguageMode.ConstrainedLanguage)
{
ThrowTerminatingError(
new ErrorRecord(
new PSNotSupportedException(AddTypeStrings.CannotDefineNewType), "CannotDefineNewType", ErrorCategory.PermissionDenied, null));
}

// Generate an error if they've specified an output
// assembly type without an output assembly
if (String.IsNullOrEmpty(outputAssembly) && outputTypeSpecified)
Expand All @@ -787,6 +780,37 @@ protected override void EndProcessing()
ThrowTerminatingError(errorRecord);
return;
}

PopulateSource();
}

// We only keep the code for backward compatibility.
internal void PopulateSource()
{
// Prevent code compilation in ConstrainedLanguage mode
if (SessionState.LanguageMode == PSLanguageMode.ConstrainedLanguage)
{
ThrowTerminatingError(
new ErrorRecord(
new PSNotSupportedException(AddTypeStrings.CannotDefineNewType), "CannotDefineNewType", ErrorCategory.PermissionDenied, null));
}

// Load the source if they want to load from a file
if (String.Equals(ParameterSetName, "FromPath", StringComparison.OrdinalIgnoreCase) ||
String.Equals(ParameterSetName, "FromLiteralPath", StringComparison.OrdinalIgnoreCase)
)
{
sourceCode = "";
Copy link
Contributor

@PaulHigin PaulHigin Nov 2, 2017

Choose a reason for hiding this comment

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

Don't we want to use the updated StringBuilderCode here to get the perf benefit? Also there is duplicate code that could be handled with base class helper methods. But I believe you are looking to refactor this code so it may not be worth changing now just to be refactored later.

Copy link
Collaborator Author

@iSazonov iSazonov Nov 3, 2017

Choose a reason for hiding this comment

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

I believe the method is a candidate to remove. I've tried add [Obsolete] but it don't work. I think we could document the method and perhaps the class as deprecated - I'll wonder if anybody use it. Another argument for removing this code is that the code is the remainder of the CodeDom.
So I split the code (no helper methods) to easily remove the deprecated code later.

Copy link
Contributor

@PaulHigin PaulHigin Nov 3, 2017

Choose a reason for hiding this comment

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

Ok.

Copy link
Member

@daxian-dbw daxian-dbw Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think the breaking changes to AddTypeCommandBase is most likely safe. I searched on Github and don't see the type AddTypeCommandBase gets used except AddType.
Given that, would it be OK that we just keep the breaking change made in #5243? Maybe you want to do further cleanup, such as completely remove the base type, in your subsequent work on Add-Type.

Copy link
Collaborator Author

@iSazonov iSazonov Nov 3, 2017

Choose a reason for hiding this comment

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

I don't see the need for AddTypeCommandBase. If the team approves, I'll be happy to remove that.

Copy link
Member

@daxian-dbw daxian-dbw Nov 3, 2017

Choose a reason for hiding this comment

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

I'm merging this PR. We can loop in the powershell committee in your subsequent add-type refactoring PR (or the issue for that work).

Copy link
Collaborator Author

@iSazonov iSazonov Nov 4, 2017

Choose a reason for hiding this comment

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

The request is in #5158.

foreach (string file in paths)
{
sourceCode += System.IO.File.ReadAllText(file) + "\n";
}
}

if (String.Equals(ParameterSetName, "FromMember", StringComparison.OrdinalIgnoreCase))
{
sourceCode = GenerateTypeSource(typeNamespace, Name, sourceCode, language);
}
}

internal void HandleCompilerErrors(AddTypeCompilerError[] compilerErrors)
Expand Down Expand Up @@ -891,7 +915,30 @@ public sealed class AddTypeCommand : AddTypeCommandBase
/// </summary>
protected override void EndProcessing()
{
base.EndProcessing();
// Prevent code compilation in ConstrainedLanguage mode
if (SessionState.LanguageMode == PSLanguageMode.ConstrainedLanguage)
{
ThrowTerminatingError(
new ErrorRecord(
new PSNotSupportedException(AddTypeStrings.CannotDefineNewType), "CannotDefineNewType", ErrorCategory.PermissionDenied, null));
}

// Generate an error if they've specified an output
// assembly type without an output assembly
if (String.IsNullOrEmpty(outputAssembly) && outputTypeSpecified)
{
ErrorRecord errorRecord = new ErrorRecord(
new Exception(
String.Format(
CultureInfo.CurrentCulture,
AddTypeStrings.OutputTypeRequiresOutputAssembly)),
"OUTPUTTYPE_REQUIRES_ASSEMBLY",
ErrorCategory.InvalidArgument,
outputType);

ThrowTerminatingError(errorRecord);
return;
}

if (loadAssembly)
{
Expand Down