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

Change to Read/Write Neutral Progress Messages for Web Cmdlets WriteToStream() #5078

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

markekraus
Copy link
Contributor

Closes #3677

The progress message can be a bit confusing depending on the context in which WriteToStream() is called. This PR changes the wording to be Read/Write neutral favoring "Processing" instead.

@markekraus markekraus changed the title Create Read/Write neutral progress messages Change to Read/Write Neutral Progress Messages for Web Cmdlets WriteToStream() Oct 10, 2017
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment

@@ -184,13 +184,13 @@
<value>Unable to retrieve certificates because the thumbprint is not valid. Verify the thumbprint and retry. </value>
</data>
<data name="WriteRequestComplete" xml:space="preserve">
<value>Writing web request completed. (Number of bytes remaining: {0})</value>
<value>Web request processing completed. (Number of bytes remaining: {0})</value>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

<value>Web request completed. (Number of bytes processed: {0})</value> 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. fixed.

</data>
<data name="WriteRequestProgressStatus" xml:space="preserve">
<value>Writing request stream... (Number of bytes written: {0})</value>
<value>Processing... (Number of bytes processed: {0})</value>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove repeating "Processing" - "processed".
Maybe:

<value>Number of bytes processed: {0}</value> 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. fixed.

@iSazonov iSazonov self-assigned this Oct 10, 2017
@@ -184,13 +184,13 @@
<value>Unable to retrieve certificates because the thumbprint is not valid. Verify the thumbprint and retry. </value>
</data>
<data name="WriteRequestComplete" xml:space="preserve">
<value>Writing web request completed. (Number of bytes remaining: {0})</value>
<value>Web request completed. (Number of bytes processed: {0})</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the original "remaining" correct? Meaning that it should be "Number of bytes to process"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was wrong all along. This string is the message that is sent last when the stream has completed.

if (cmdlet != null)
{
ProgressRecord record = new ProgressRecord(ActivityId,
WebCmdletStrings.WriteRequestProgressActivity,
StringUtil.Format(WebCmdletStrings.WriteRequestComplete, totalWritten));
record.RecordType = ProgressRecordType.Completed;
cmdlet.WriteProgress(record);
}

in 5.1:

$Job = Start-Job -ScriptBlock { 
    Invoke-WebRequest https://dist.nuget.org/win-x86-commandline/latest/nuget.exe -outfile $env:temp\nuget.exe 
} | Wait-Job
$progress = $Job.ChildJobs[0].Progress.ReadAll()
$progress[-2].StatusDescription
$progress[-1].StatusDescription

result:

Writing request stream... (Number of bytes written: 5010552)
Writing web request completed. (Number of bytes remaining: 5010552)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, your change looks good then. Thanks!

@iSazonov iSazonov merged commit f6864c3 into PowerShell:master Oct 11, 2017
@markekraus markekraus deleted the WriteToStream branch January 19, 2018 19:00
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.

None yet

3 participants