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.

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

Issues with AboutEnumeration #372

Closed
ghost opened this issue Mar 5, 2020 · 8 comments · Fixed by #374
Closed

Issues with AboutEnumeration #372

ghost opened this issue Mar 5, 2020 · 8 comments · Fixed by #374
Labels
Category-Koans Invoking the Great Doubt Issue-Bug 🐛 Something's wrong!

Comments

@ghost
Copy link

ghost commented Mar 5, 2020

First issue

Issue #369, still seems to be broken, I tried the Update-Koans, I even copied directly from the Git Repository, still broken. So to fix "You are already in a test case." issue

At line 112 add a "}"
At line 628 remove a "}"

That was lots of fun to find! (head shaking)

Second Issue

The following code, found at line 228, is not going to compile unless you are using Powershell Core 6.2.0 or better, and the Skip command in the test doesn't keep it from compiling

enum Int64Enum : Int64 { LargeValue = 9223370000000000000 }

I couldn't get it to work with Powershell Core 7.0 either, but I didn't try very hard.

I simply removed the test "It 'can represent other integer in PowerShell Core'" from the Koan and kept on rolling.

It is a cool concept that you are introducing but unless you are going to force using Powershell Core 6.2.0 or better, it may not be worth hanging on to.

Third Issue

At line 286, the [Enum]::TryParse is being used incorrectly, the $enumType is not needed
The code should be corrected to
$result = [Enum]::TryParse( $valueToParse, [Ref]$parseResult)

@ghost ghost added Category-Koans Invoking the Great Doubt Issue-Discussion Let's talk about it! labels Mar 5, 2020
@vexx32
Copy link
Owner

vexx32 commented Mar 6, 2020

First issue

Yep, that's... wow, that looks like a real pain to find. Well done, and thankyou! I'll get that one fixed.

Second issue

Yeah, I see the logic error there. That should be a string, not a scriptblock, and convert just before it's used.

Third issue

Hmm... I don't think so. The overload definitions for [Enum]::TryParse() are:

OverloadDefinitions
-------------------
static bool TryParse(type enumType, string value, [ref] System.Object result)
static bool TryParse(type enumType, string value, bool ignoreCase, [ref] System.Object result)
static bool TryParse[TEnum](string value, [ref] TEnum result)
static bool TryParse[TEnum](string value, bool ignoreCase, [ref] TEnum result)

The overload you're suggesting we use there is a generic method, and that call is very fragile -- if a variable containing a value of the wrong type is used as the [ref] (including a variable that has no value / is set to $null), that's not going to be resolvable. PowerShell doesn't have an explicit way to specify the generic parameter for the method to ensure it resolves correctly.

Specifying the type with the other overload is a better bet, I think; it's too easy to have it fail otherwise. It's not incorrect, by any means, it's just a different overload for the method. 🙂

@vexx32 vexx32 added Issue-Bug 🐛 Something's wrong! and removed Issue-Discussion Let's talk about it! labels Mar 6, 2020
vexx32 added a commit that referenced this issue Mar 6, 2020
vexx32 added a commit that referenced this issue Mar 6, 2020
Using a raw scriptblock will break the parser in earlier versions.

Fix is to define it as a string and parse that just before execution.
@vexx32
Copy link
Owner

vexx32 commented Mar 6, 2020

Hmm, looks like the additional overloads are indeed missing in Windows PowerShell. I wasn't aware they were actually quite new. I'll have a look at what we can do there...

@vexx32
Copy link
Owner

vexx32 commented Mar 6, 2020

@indented-automation do you have any suggestions on that last point?

Code is here:

It 'can use the enum type to test if a value exists in the enum' {
<#
The Enum type includes two static methods:
Parse
TryParse
Parse is normally used to convert a string into an enum value. Parse is not required
in PowerShell as values can be directly cast to the enum type as shown in the first example.
TryParse returns true if a value was successfully parsed, and false otherwise. TryParse
expects three arguments:
1. The enum type.
2. The string value to parse.
3. A reference to a variable which can hold the parse result.
Running [Enum]::TryParse without parentheses will show the arguments the method expects.
#>
$valueToParse = '____'
$enumType = [DayOfWeek]
$parseResult = [DayOfWeek]::Sunday
$result = [Enum]::TryParse($enumType, $valueToParse, [Ref]$parseResult)
$result | Should -BeTrue
$parseResult | Should -Not -Be 'Sunday'
}
}

The two overloads that take a specific [type] object in .NET Core are missing in Windows PowerShell. 😕

@ghost
Copy link
Author

ghost commented Mar 6, 2020

I agree that the untyped TryParse is fragile, I suppose that I should have mentioned that I had gone looking for the additional overload definitions, and was unable to locate them. I suppose this is the excitement of working through these things, when the environment is so highly dynamic.

Looking forward to seeing what @indented-automation has to say.

@indented-automation
Copy link
Contributor

PS is bit to free with how it binds arguments. Was sure I'd used that method before in Win PS... apparently not.

It can be swapped to the more specific [DayOfWeek]::TryParse which only needs two arguments. I'm away this weekend, I can fix it up early next week?

Chris

@vexx32
Copy link
Owner

vexx32 commented Mar 6, 2020

That's one option, but I don't think that TryParse method is any more specific... I had a quick look at that, but unless I missed something, it looks like it just inherits the same fragile methods from Enum itself.

That said, provided the variable you're passing in as reference is of the right Enum type, it'll still work okay.

vexx32 added a commit that referenced this issue Mar 6, 2020
* ♻️ Cleanup old test file

Refactor slightly, match file more to current style.

Rename a few tests to make more sense.

* ✅ Add regression test

We can test file ASTs during our static analysis checks.

Added a test to check we don't have any nested It blocks in the file.

* 🐛 (#372) Fix missing / extra brace

* 📝 🎨 Adjust AboutEnumeration formatting

* 🐛 (#372) Delay parsing of enum

Using a raw scriptblock will break the parser in earlier versions.

Fix is to define it as a string and parse that just before execution.
@indented-automation
Copy link
Contributor

That part of the example was fine, I obviously only tested it properly on PS Core which is why we're here :)

$DayOfWeek = [DayOfWeek]::Sunday
[DayOfWeek]::TryParse('Wednesday', [Ref]$DayOfWeek)

This at least works in Win PS compared with the original :-D

@vexx32
Copy link
Owner

vexx32 commented Mar 6, 2020

Might be worth an added note that you have to pass in a variable that already contains one of the enum values for it to work as well, but yep! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category-Koans Invoking the Great Doubt Issue-Bug 🐛 Something's wrong!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants