Skip to content

Reuse ProgressRecord in Web Cmdlets - #3411

Merged
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
iSazonov:recordprogress
Mar 28, 2017
Merged

Reuse ProgressRecord in Web Cmdlets#3411
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
iSazonov:recordprogress

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Mar 24, 2017

Copy link
Copy Markdown
Collaborator

Fix #3367

@iSazonov Ilya (iSazonov) added the Review - Needed The PR is being reviewed label Mar 24, 2017
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Mar 24, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/StreamHelper.cs Outdated
{
long totalLength = 0;
byte[] buffer = new byte[StreamHelper.ChunkSize];
ProgressRecord record = new ProgressRecord(StreamHelper.ActivityId, WebCmdletStrings.ReadResponseProgressActivity, StringUtil.Format(WebCmdletStrings.ReadResponseProgressStatus, 0));

@daxian-dbw Dongbo Wang (daxian-dbw) Mar 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

StringUtil.Format(WebCmdletStrings.ReadResponseProgressStatus, 0)

This is not necessary here. How about just new ProgressRecord(StreamHelper.ActivityId, WebCmdletStrings.ReadResponseProgressActivity, "statusDescriptionPlaceholder"). Then "statusDescriptionPlaceholder" would be a constant.

@iSazonov Ilya (iSazonov) Mar 25, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but we cannot use String.Empty - ProgressRecord don't allow. So if we want a const it may be " " (single space). Is it well?
new ProgressRecord(StreamHelper.ActivityId, WebCmdletStrings.ReadResponseProgressActivity, " ")
or
new ProgressRecord(StreamHelper.ActivityId, WebCmdletStrings.ReadResponseProgressActivity, singleSpace) string static singleSpace = " ";

@daxian-dbw Dongbo Wang (daxian-dbw) Mar 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about just using the string statusDescriptionPlaceholder as the placeholder?

@iSazonov Ilya (iSazonov) Mar 27, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit b212c7a into PowerShell:master Mar 28, 2017
@iSazonov
Ilya (iSazonov) deleted the recordprogress branch March 29, 2017 05:55
@iSazonov Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 29, 2017
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @daxian-dbw
Reuse ProgressRecord in Web Cmdlets (PowerShell#3411)
665b3d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iSazonov @daxian-dbw @msftclas