Do not require activity when creating a completed progress record

[MartinGC94]

PR Summary

Fixes#15252
Allows you to complete progress records with Write-Progress without specifying an activity.
This is done by using a predefined string in the activity for Complete records because the API expects a non-empty string and updating the API would cause issues in remoting scenarios with a different PowerShell host.

PR Context

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed: Add note for Write-Progress that the Activity parameter is optional MicrosoftDocs/PowerShell-Docs#9864
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[MartinGC94]

It doesn't seem possible to fix this with parametersets without making a breaking change.
If there's no default parameterset set then the parameter binding can't resolve the parameterset in scenarios like Write-Progress -Activity "Something" -Id 1 -Completed.
If it's set to the current parameterset then it will prompt for an Activity value for Write-Progress -Completed.
If it's set to the new parameterset then it will prompt for a Completed value for Write-Progress -Activity "Something"

It seems like the only option left is to keep the current parameterset and simply make Activity optional, then if no value is provided either provide a default value, or throw an error unless it's a Completed progress record.

[SteveL-MSFT]

I'll queue this up for discussion on Cmdlets WG

[SteveL-MSFT]

This script example seems to work?

[cmdletbinding(DefaultParameterSetName='two')] param( [parameter(ParameterSetName='one',mandatory=$true)] [parameter(ParameterSetName='two',mandatory=$false)] [string]$activity, [parameter(ParameterSetName='two')] [switch]$completed ) $activity$completed

[MartinGC94]

That's not an accurate example though. Completed still needs to exist in the old parameter set so we don't break backwards compatibility with scripts like Write-Progress -Activity bla -SecondsRemaining 0 -Completed.

This example is more accurate:

function Write-Progress2{[CmdletBinding(DefaultParameterSetName = "two")] Param ( [parameter(ParameterSetName='one', mandatory=$true)] [parameter(ParameterSetName='two', mandatory=$false)] [string]$activity, [parameter(ParameterSetName='one', Mandatory = $false)] [parameter(ParameterSetName='two', Mandatory = $false)] [switch]$completed ) $PSCmdlet.ParameterSetName } 

but by making parameter set "two" the default set and the "completed" switch optional we allow people to call it without any parameters at all: Write-Progress2. We can fix that by making the switch mandatory but then calling it with Write-Progress2 -Activity bla will default to set 2 and prompt for a value for the "completed" parameter.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

[SteveL-MSFT]

@PowerShell/wg-powershell-cmdlets reviewed this. We agreed to make -Activity not mandatory and allow null/empty value. We understand that new scripts using this will cause older PowerShell to prompt to fill in this parameter, but proper documentation can alleviate this.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

[doctordns]

The Cmdlet working group has reviewed this and we recognize the benefit of the requested behavior, and we have seen other scenarios where omitting activity and status is useful.
However, we believe a more elegant change is to add an overloaded constructor for ProgressRecord to allow only activityId.
Further, the Write-Progress cmdlet should be updated to make activity a non-mandatory parameter and call the appropriate constructor,
with a runtime check in set_Activity when the Completed parameter is used and produce an error if Completed is not used and activity is not provided

[MartinGC94]

with a runtime check in set_Activity when the Completed parameter is used and produce an error if Completed is not used and activity is not provided

I don't understand where you want this check. I can't find set_Activity anywhere in the solution. Do you mean inside the cmdlet code itself?
Also, I feel like this comment was forgotten: #18474 (comment) basically the suggested approach will cause problems in remoting scenarios from old -> new and custom hosts may have expectations that Activity is not null.

[SteveL-MSFT]

@MartinGC94 you are correct, when we discussed this in the WG, I had forgotten about the remoting scenario. Let me bring it up to the WG one more time. Sorry about this.

[ghost]

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

[JamesWTruher]

The WG hasn't changed our mind about our earlier opinion, but we believe that the serialization of the progress record could be altered so it ensures that a space would be provided during the serialization process instead of changing the default value of the parameter. It looks like this could be done around line 486 in System.Management.Automation/engine/ProgressRecord.cs.

[MartinGC94]

I've applied the WG suggestions with a slight modification: I added the runtime check to ProcessRecord instead of the setter for Activity because parameters can be bound in any order so we can't know for certain if the Completed parameter is set when binding Activity.

[SteveL-MSFT]

Sorry for the back-and-forth on this, but we wanted to make sure we have the right long term design as well as addressing the app-compat issue. This looks good to me. Thanks!

[pull-request-quantifier-deprecated]

This PR has 28 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label : Extra Small Size : +23 -5 Percentile : 11.2% Total files changed: 3 Change summary by file extension: .cs : +22 -4 .ps1 : +1 -1 

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detected.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[JamesWTruher]

lgtm

[ghost]

🎉v7.4.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes