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

Small fixes for typos and clarity #387

Merged
merged 9 commits into from
Jun 14, 2020
Merged

Conversation

devlincashman
Copy link
Contributor

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

  • Pull Request has a meaningful title.
  • Summarised changes.
  • Pull Request is ready to merge & is not WIP.
  • Added tests / only testable interactively.
    • Make sure you add a new test if old tests do not effectively test the code changed.
  • Added documentation / opened issue to track adding documentation at a later date.

Copy link
Owner

@vexx32 vexx32 left a 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'
Copy link
Owner

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']
Copy link
Owner

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:

  1. contain at least one key name that is Carrots
  2. have at least one of the values be 'Fruit'
  3. the key Oranges should map to the value 'Fruit'
  4. the value for Carrots should not be the same as the value for Oranges

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 __
Copy link
Owner

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

Comment on lines +70 to +86
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'
Copy link
Owner

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 :)

@vexx32 vexx32 added Category-Koans Invoking the Great Doubt PR-Awaiting-Author ✏️ Waiting on a response from the user who submitted the PR. labels May 10, 2020
@vexx32 vexx32 self-assigned this May 10, 2020
@vexx32
Copy link
Owner

vexx32 commented Jun 5, 2020

@devlincashman are you still looking to continue / finish this one up? 🙂

@vexx32 vexx32 changed the base branch from master to main June 13, 2020 01:36
@vexx32 vexx32 removed the PR-Awaiting-Author ✏️ Waiting on a response from the user who submitted the PR. label Jun 14, 2020
Copy link
Owner

@vexx32 vexx32 left a 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! 😊 💖

@vexx32 vexx32 merged commit 874f27c into vexx32:main Jun 14, 2020
@vexx32 vexx32 mentioned this pull request Jun 14, 2020
5 tasks
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants