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

Clarify AboutPSProviders->FileSystemProvider Koan. #189

Merged
merged 2 commits into from Jun 3, 2019
Merged

Clarify AboutPSProviders->FileSystemProvider Koan. #189

merged 2 commits into from Jun 3, 2019

Conversation

ghost
Copy link

@ghost ghost commented May 26, 2019

PR Summary

Resolves #174

Instead of using the TempDrive created by Pester, added logic to determine execution environment and use the local temp directory.

Instead of dynamically creating a text file, replaced with static lines of text. Makes the following tests clearer.

Context

Changes

  • Replaced dynamically generated file with static text.

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.

@ghost ghost requested review from steviecoaster and vexx32 as code owners May 26, 2019 02:50
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.

I like the look of this so far!

We may need to change tack a bit, as I'd like to keep boilerplate code out of the koan files themselves as much as we can.

PSKoans/Koans/Cmdlets 1/AboutPSProviders.Koans.ps1 Outdated Show resolved Hide resolved
PSKoans/Koans/Cmdlets 1/AboutPSProviders.Koans.ps1 Outdated Show resolved Hide resolved
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.

Looking pretty good, just want to make sure we're headed the right direction here. Appreciate your efforts so far! 😊

PSKoans/Koans/Cmdlets 1/AboutPSProviders.Koans.ps1 Outdated Show resolved Hide resolved
PSKoans/Private/Get-TempDirectory.ps1 Outdated Show resolved Hide resolved
@vexx32
Copy link
Owner

vexx32 commented Jun 2, 2019

🤔 Hmm, looks like you pulled in some commits with a merge or something? You might need to rebase -i master or something and trim those out. 🙂

@nohwnd
Copy link
Contributor

nohwnd commented Jun 2, 2019

Switch to this branch. Then do git branch "file-system-provider-backup". This will checkout another branch from this branch to make sure you don't lose the commits. Then do git reset head~3 to reset the last 3 commits, then git stash -u and git stash drop to throw away the changes coming from the last three commits. if you now do git log you should see the commits first two commits but not the last three. once you are happy git push --force-with-lease to update the branch on the remote, which will also update this merge request. (or you can do rebase but that would mean that you'd need to solve merge conflicts if there were any, so this approach seems easier :D)

@vexx32
Copy link
Owner

vexx32 commented Jun 2, 2019

Yeah but then he'd lose also his actual last commit. Rebase is simpler in my mind... Eh, whatever works. :P

@nohwnd
Copy link
Contributor

nohwnd commented Jun 2, 2019

Ah okay, there we commits to keep. In that case interactive rebase is indeed simpler.

@ghost
Copy link
Author

ghost commented Jun 2, 2019

@vexx32 Yeah, instead of doing git pull like i should have to update my local branch to the changes of the project master, I did git fetch then git rebase FETCH_HEAD thinking I would be able to skip a merge commit. Long story short, just being a dummy. At this point, I think the issue related to this Pull Request is actually resolved AND git history is coherent. Let me know if you want anything done differently.

@nohwnd I haven't used git push --force-with-lease before / didn't know it existed. Thanks for the advanced! Worked well.

vexx32
vexx32 previously approved these changes Jun 2, 2019
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.

Looks good! Minor nit but otherwise we're all good.

Thanks so much! 😄

PSKoans/Koans/Cmdlets 1/AboutPSProviders.Koans.ps1 Outdated Show resolved Hide resolved
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.

Love it, thank you!

@vexx32 vexx32 merged commit 1edc1b9 into vexx32:master Jun 3, 2019
@vexx32 vexx32 added the Category-Koans Invoking the Great Doubt label Jun 11, 2019
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.

Issues with 'Filesystem Provider'
2 participants