-
-
Notifications
You must be signed in to change notification settings - Fork 177
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
Small fixes for typos and clarity #387
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.
Thanks for coming back through and helping make these better! 😊 I have a few comments and adjustments for us to look a bit closer at.
I appreciate you taking the time to tweak these!
@@ -134,7 +134,7 @@ Describe 'Environment Provider' { | |||
|
|||
Describe 'FileSystem Provider' { | |||
BeforeAll { | |||
$Path = 'TestDrive:' | Join-Path -ChildPath 'File001.tmp' | |||
$Path = 'TEMP:' | Join-Path -ChildPath 'File001.tmp' |
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.
TEMP:
as a pre-existing PSDrive doesn't exist until PS6 or 7 I think?
TestDrive:
is provided by Pester and cleans up after itself without us having to do any manual work to do so. Was this one not working for you? 🙂
@@ -180,7 +180,7 @@ Describe 'Hashtables' { | |||
$Hashtable.ContainsValue('Fruit') | Should -BeTrue | |||
|
|||
$Hashtable['Oranges'] | Should -Be 'Fruit' | |||
$Hashtable['Carrots'] | Should -Not -Be $Hashtable['Oranges'] | |||
$Hashtable['Carrots'] | Should -Be $Hashtable['Oranges'] |
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.
This might need more clarification, for sure, but I don't think this is the problem here.
The conditions as set without changing this line are that the hashtable must:
- contain at least one key name that is
Carrots
- have at least one of the values be
'Fruit'
- the key
Oranges
should map to the value'Fruit'
- the value for
Carrots
should not be the same as the value forOranges
The intended solution is for carrots to be marked as 'Vegetables'
when adding keys to the hashtable. 🙂
Is there a way we can make that a bit more clear for this koan?
@@ -125,7 +125,7 @@ Describe 'Pipelines and Loops' { | |||
#> | |||
$i | |||
} | |||
$Values | Should -Be @(0, 1, 2, 3, 4) | |||
$Values | Should -Be __ |
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.
Hmm. I tend to prefer that blanks take a similar form to what's expected; we're expecting an array here, so a single blank is a bit misleading imo.
Perhaps something like this would be better?
$ExpectedValues = @(
__
__
__
__
__
)
$ExpectedValues | Should -Be $Values
Result = 'aacddehhlllooorttuWwy' | ||
} | ||
@{ | ||
String = 'Out of nowhere, the mind comes forth.' | ||
Result = 'cdeeeeffhhhimmnnooOoorrstttuw' | ||
Result = 'cdeeeeffhhhimmnnOoooorrstttuw' | ||
} | ||
@{ | ||
String = 'Because it is so very clear, it takes longer to come to the realization.' | ||
Result = 'aaaaaBccceeeeeeeeeghiiiiiklllmnnoooooorrrrsssstttttttuvyz' | ||
} | ||
@{ | ||
String = 'The hands of the world are open.' | ||
Result = 'aaddeeeefhhhlnnoooprrstTw' | ||
Result = 'aaddeeeefhhhlnnoooprrsTtw' | ||
} | ||
@{ | ||
String = 'You are those huge waves sweeping everything before them, swallowing all in their path.' | ||
Result = 'aaaaabeeeeeeeeeeeefgggghhhhhhiiiiillllmnnnnoooopprrrrsssstttttuuvvwwwwyY' | ||
Result = 'aaaaabeeeeeeeeeeeefgggghhhhhhiiiiillllmnnnnoooopprrrrsssstttttuuvvwwwwYy' |
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.
Can you share the code you used to get these results?
I remember what I did to get the result I laid out in there, but there's a good chance it didn't play nice with the upper and lower cases. This looks right, just want to verify :)
@devlincashman are you still looking to continue / finish this one up? 🙂 |
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.
Approving for now, will address changes from review in follow up PR. Thanks for your contribution @devlincashman! 😊 💖
PR Summary
Context
I've been completeling the Koans and along the way trying to fix any errors I happen to catch. Most of the changes I've done are for broken tests, but I did change a few things for clarity that caught me up.
I asked in the #bridge PS channel about a few of these like the sort method for the kata, so if my changes were incorrect there sorry about that.
Also wanted to say thanks so much for all the hard work you do on this project Joel! And of course everyone else who contributes.
See you in chat!
Changes
Updated TestDrive: to TEMP: in PSProviders as TestDrive: is not a valid drive from what I could tell?
Changed several arrays, Pester tests that were failing and seemed likely to be an error.
Sorting Kata has capitals in the sorted characters at different points, so a simple sort gets an error. Assuming you are not asking for a merge sort which seems advanced. Simplified test strings to pass a basic sort.
Checklist