Skip to content

Conversation

@schuelermine
Copy link
Contributor

Another fix for #15490. For your consideration.

Previously, the command sometimes did declare SupportsShouldProcess, but didn’t actually implement it.

Because of the fact that Start-NativeExecution is used with complex script blocks, it wasn’t possible to simply implement SupportsShouldProcess in Start-NativeExecution.
By implementing the ShouldProcess logic for each case individually, we’re also able to provide more helpful messages to the user.

I have not implemented SupportsShouldProcess for the other scripts that are called by this function. I do not have the patience, and I do not believe it is worth it.

In particular, I think that this function is in dire need of -WhatIf support, since its name (“Start-PSBootstrap”) doesn’t initially suggest to the user that (potentially irreversible) changes to the system configuration will be made. This is not the case with things like Install-Dotnet and install-powershell.ps1, which have far more revealing names.

I believe the current code is pretty DRY. Maybe the call to $PSCmdlet.ShouldProcess could be implemented in a subroutine, since text is often repeated?
I’ve also made some changes in some places to make the code better suitable for this feature. In particulare, the Gem installation routine is now run using a loop over a table.

In looking at the code, I have come to question the necessity of many of the calls to Start-NativeExecution. It’s unclear what error handling it provides and if it should be used to run more complex script blocks.

AMEND:

I have tested this code on Pop!_OS 20.10. I can’t test it on Windows since I don’t believe my Windows license is valid anymore, and I’m not in the mood to comit crimes for software development. This is of course suboptimal as the parent commits don’t seem to pass the unit tests either.
However, since much of the code is simply repeated with only messages swapped out, I feel that it is likely to work.

I haven’t tested the code rigorously. Please treat with caution.

Previously, the command sometimes did declare SupportsShouldProcess, but didn’t actually implement it.

Because of the fact that Start-NativeExecution is used with complex script blocks, it wasn’t possible to simply implement SupportsShouldProcess in Start-NativeExecution.
By implementing the ShouldProcess logic for each case individually, we’re also able to provide more helpful messages to the user.

I have not implemented SupportsShouldProcess for the other scripts that are called by this function. I do not have the patience, and I do not believe it is worth it.

In particular, I think that this function is in dire need of -WhatIf support, since its name (“Start-PSBootstrap”) doesn’t initially suggest to the user that (potentially irreversible) changes to the system configuration will be made. This is not the case with things like Install-Dotnet and install-powershell.ps1, which have far more revealing names.

I believe the current code is pretty DRY. Maybe the call to $PSCmdlet.ShouldProcess could be implemented in a subroutine, since text is often repeated?
I’ve also made some changes in some places to make the code better suitable for this feature. In particulare, the Gem installation routine is now run using a loop over a table.

In looking at the code, I have come to question the necessity of many of the calls to Start-NativeExecution. It’s unclear what error handling it provides and if it should be used to run more complex script blocks.

 AMEND:

I have tested this code on Pop!_OS 20.10. I can’t test it on Windows since I don’t believe my Windows license is valid anymore, and I’m not in the mood to comit crimes for software development. This is of course suboptimal as the parent commits don’t seem to pass the unit tests either.
However, since much of the code is simply repeated with only messages swapped out, I feel that it is likely to work.

I haven’t tested the code rigorously. Please treat with caution.
@schuelermine
Copy link
Contributor Author

I don't think this is worth keeping open, given the progress on the other one.
Also, the Implementation is suboptimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants