GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Remove WriteVerbose statement from ConvertTo-Json by devblackops · Pull Request #7487 · PowerShell/PowerShell

Conversation

@devblackops

@devblackops Brandon Olin (devblackops) commented Aug 9, 2018 •
edited by daxian-dbw
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

Fix #7486.

In PR #6392, a WriteVerbose statement was added to ConvertTo-Json which I feel is unnecessary. On large or complex objects being serialized to json, this produces a ton of verbose messages which isn't adding any value to the end user. This PR removes that statement.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • 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: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link: Remove unnecessary verbose message from ConvertTo-Json  #7486
  • Testing - New and feature
    • Not Applicable 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
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@devblackops
Remove unnecessary verbose output to reduce noise
dec19a8
@devblackops
Brandon Olin (devblackops) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners August 9, 2018 03:57
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Mark Kraus (markekraus) August 9, 2018 09:24
@iSazonov

Ilya (iSazonov) commented Aug 9, 2018

Copy link
Copy Markdown
Collaborator

Brandon Olin (@devblackops) I wonder do you see the verbose messages without -Verbose ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markekraus
Mark Kraus (markekraus) suggested changes Aug 9, 2018
View reviewed changes

@markekraus Mark Kraus (markekraus) left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Brandon Olin (@devblackops) Please also remove the message string which would no longer be used after this pr:

PowerShell/src/Microsoft.PowerShell.Commands.Utility/resources/UtilityCommonStrings.resx

Lines 177 to 179 in 791159d

<data name="ConvertToJsonProcessValueVerboseMessage" xml:space="preserve">
<value>Processing object of type [{0}] at depth {1}</value>
</data>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@devblackops

Brandon Olin (devblackops) commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) The messages aren't shown without -Verbose but often will be inherited from cmdlets/functions that invoke ConvertFrom-Json that do have -Verbose specified.

It's easy enough to suppress the verbose output with -Verbose:False or set it via $PSDefaultParameterValues but in this case, these messages aren't really adding any value, hence the PR to remove them.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Brandon Olin
Remove unused resource string
fcdcafb
@devblackops

Brandon Olin (devblackops) commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Mark Kraus (@markekraus) I pushed a commit to remove the resource string.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw Dongbo Wang (daxian-dbw) self-assigned this Aug 9, 2018
dantraMSFT
Dan Travison (dantraMSFT) approved these changes Aug 9, 2018
View reviewed changes

@dantraMSFT Dan Travison (dantraMSFT) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I agree the verbose statement doesn't add much value.

It was added when StopProcessing was added so I expect it was done for debugging purposes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markekraus
Mark Kraus (markekraus) approved these changes Aug 9, 2018
View reviewed changes

@markekraus Mark Kraus (markekraus) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iSazonov
Ilya (iSazonov) approved these changes Aug 10, 2018
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 8374a5c into PowerShell:master Aug 10, 2018
@iSazonov

Ilya (iSazonov) commented Aug 10, 2018

Copy link
Copy Markdown
Collaborator

Brandon Olin (@devblackops) Thanks for your contribution!
devblackops reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) mentioned this pull request Sep 18, 2018
Add EscapeHandling parameter in ConvertTo-Json cmdlet #7775
Merged
8 of 11 tasks
xtqqczze added a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Jan 14, 2019
@xtqqczze
Revert "Remove one unneeded verbose output from 'ConvertTo-Json' (Pow…
232b48b 
…erShell#7487)"

This reverts commit 8374a5c.
@xtqqczze xtqqczze mentioned this pull request Jan 14, 2019
Fix ConvertTo-Json test #8645
Closed
8 of 11 tasks
@devblackops
Brandon Olin (devblackops) deleted the fix/converttojson-verbose branch January 15, 2019 21:41
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@devblackops @daxian-dbw
Remove one unneeded verbose output from 'ConvertTo-Json' (PowerShell#…
2967f06 
…7487)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
+2 more reviewers
@markekraus Mark Kraus (markekraus) markekraus approved these changes
@dantraMSFT Dan Travison (dantraMSFT) dantraMSFT approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Remove unnecessary verbose message from ConvertTo-Json

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@devblackops @iSazonov @markekraus @dantraMSFT @daxian-dbw
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.