Adding try/catch logic to Write-VcsStatus

[paulmarsy]

Adding try/catch logic to Write-VcsStatus so that if one of the scriptblocks that has been added to the VcsPromptStatuses array fails the other scriptblocks are not affected (are still invoked)

[paulmarsy]

I've created two separate pull requests as they solve different bugs despite being in the same area of code, so not sure if you would want to accept none/one/both or they have different feedback and changes needed

[dahlbyk]

Sorry for the ridiculous delay in looking at these... I like the try/catch to make Write-VcsStatus less destructive, but have concerns about the extra change for #169. If you can tease the changes apart, we can get this in first.

Once we get this nailed down, you might consider proposing an equivalent change to posh-hg.

[dahlbyk]

Out of curiosity, why New-Variable ... versus the simpler $Global:VcsPromptStatuses = ...?

[paulmarsy]

This relates more to the other proposed change, it was so the name of the global variable is controlled & set in one location, currently the $VcsPromptStatusesVariableName variable but potentially it could be set/changed from $GitPromptSettings

[paulmarsy]

My mistake, I thought I had separated the two as they were done in separate branches.
I'll update this so it is purely the try/catch logic change 😄

[paulmarsy]

The last two commits (b6dd6ea and 4754915) detect errors written to the error stream as well as terminating exceptions which are thrown.

Let me know if you'd prefer them to be removed, as they detect non-terminating errors it may be better to continue as per normal.

[dahlbyk]

@_?

[dahlbyk]

New changes look good (other than @_) - can you do me a favor and rebase on master without the unrelated commits?

[paulmarsy]

I've tweaked the history to remove the WIP & unrelated checkins so it is cleaner now 😀

I have replaced the using of the splat operator with just setting the ErrorRecord parameter to be the object.. Regardless of the type of exception thrown or what's sent to write-error they get changed into ErrorRecord objects so no need to map the object's properties onto the silent write-error call as I was doing.

For now I've left in the functionality which detects and suppresses errors written to the errors stream/use of write-error.. It does complicate the function somewhat so let me know if you'd prefer to simplify it to only catch terminating exceptions.
If that is your preference I'll leave the current commit in the history for reference in the incase it may be useful somewhere else.

Off topic but related.. Posh-git, posh-hg and posh-svn all implement the same functions, has there been any discussion or thought on a provider agnostic base containing these common hooks so each provider repo can focus on its specific implementation details?

[paulmarsy]

Oops - I accidentally closed the PR while updating it with the latest changes 😊

Let me know if you're happy with the proposed implementation - I am in two minds about the detection & suppression of the error stream in addition to handling thrown exceptions..
It complicates the error handling logic & function, but does provide an extra layer of error handling invoked code which can be external to posh-git..

[dahlbyk]

Posh-git, posh-hg and posh-svn all implement the same functions, has there been any discussion or thought on a provider agnostic base containing these common hooks so each provider repo can focus on its specific implementation details?

Discussion? No. I tried to keep the Write-VcsStatus logic simple specifically to keep our shared surface area small, allowing independent evolution as necessary. To that end, my first instinct here is to just mandate that the subscribed status writers guarantee they won't throw so we can keep the copy-pasta simple.

That said, as a user of only posh-git I'm a very poor steward the multi-VCS situation (c.f. #111). If there's a clean way to extract a shared module on which all three could depend, I'm all for it (provided that manual, PsGet and Chocolatey installations keep working with minimal ceremony).

[dahlbyk]

@paulmarsy is this still something that you're struggling with? I don't object to the change in principle, but I let it get way out of date. 😞