8000 Added comma to replaced characters in assemblyname by TimCurwick · Pull Request #4136 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Added comma to replaced characters in assemblyname #4136

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

Merged
merged 9 commits into from
Jul 6, 2017
Merged

Added comma to replaced characters in assemblyname #4136

merged 9 commits into from
Jul 6, 2017

Conversation

TimCurwick
Copy link
Contributor

A script with a native class will not run if it is saved with a name or full path with a comma.

I added the comma to the list of characters that are replaced with a similar character when creating an assemblyName based on a file path. Added pester test to test.

Discussed with Jason Shirk.

@msftclas
Copy link

@TimCurwick,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot


It "Script with a class definition can run from a path without a comma" {

$FilePath = '.\MyTest.ps1'
Copy link
Member

Choose a reason for hiding this comment

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

Please use $TestDrive for temporary files.

}

catch {
$Success = $False
Copy link
Member

Choose a reason for hiding this comment

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

Instead use the Should Not Throw pattern. Example:

{ 1 | Out-File -PSPath $tempFile } | Should Not Throw

$Success | Should Be $True
}

It "Script with a class definition can run from a path with a comma" {
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above test

$Success | Should Be $True
}

It "Script with a class definition can run from a path with a comma" {
Copy link
Member

Choose a reason for hiding this comment

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

These two test cases can be combined into 1 using the -TestCases parameter for It. Example:

@@ -1113,12 +1113,13 @@ internal static Assembly DefineTypes(Parser parser, Ast rootAst, TypeDefinitionA
var definedTypes = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

// First character is a special mark that allows us to cheaply ignore dynamic generated assemblies in ClrFacede.GetAssemblies()
Copy link
Member

Choose a reason for hiding this comment

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

Typo in comment for ClrFacade.

$FilePath = '.\MyTest.ps1'

try {
'class MyClass { [string]$MyProperty }; $True' | Out-File -FilePath $FilePath
Copy link
Contributor

Choose a reason for hiding this comment

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

Use testdrive: or $testdrive - then Pester will ensure the file is deleted when the test completes.

Remove-Item -Path $FilePath
}

$Success | Should Be $True
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid Should Be $True - this results in a typically unhelpful message in the log.

I suggest a test that actually uses the class, so maybe something like:

Instead, try something like:

`@
class MyClass { static [string]$MyProperty = $PSScriptRoot }
[MyClass]::MyProperty
'@ > $testDrive/My,Test.ps1

& $testDrive/My,Test.ps1 | Should Match ".*My,Test.ps1"

Or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$PSScriptRoot doesn't work inside a class.

@@ -1,42 +1,21 @@
Describe "Script with a class definition run path" -Tags "CI" {
Describe "Script with a class definition run path" {
Copy link
Member

Choose a reason for hiding this comment

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

Missing -Tags 'CI'

}

$Success | Should Be $True
{ . $FilePath } | Should Not Throw
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not necessary - the next test covers this case.

@daxian-dbw
Copy link
Member

@adityapatwardhan and @lzybkr could you please take another look? I think the author has addressed all your comments.

@daxian-dbw daxian-dbw self-assigned this Jul 5, 2017
@daxian-dbw daxian-dbw merged commit d1e05ef into PowerShell:master Jul 6, 2017
@daxian-dbw
Copy link
Member

@TimCurwick Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0