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 — Cleanup CodeFactor issues with empty strings by iSazonov · Pull Request #6950 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented May 28, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

PR Summary

Please fast review to avoid merge conflicts because of many files changed.

Please review commit by commit - every commit contains only single kind of change so you can do very fast review.

This corrects about 1% of the CodeFactor issues.

The PR replace "" with string.Empty.

All changes is made in VS Code with Regex patterns - one pattern by commit:

'= "";' -> '= string.Empty;'

'return "";' -> 'return string.Empty;'

': "";' -> ': string.Empty;'

'\?\? "";' -> '?? string.Empty;'

'\(""\)' -> '(string.Empty)'

' ""\)' -> ' string.Empty)'

'\("",' -> '(string.Empty,'

', "",' -> ', string.Empty,'

',""\)' -> ', string.Empty)'

'\? "" :' -> '? string.Empty :'

'\?\? ""' -> '?? string.Empty'

' = "",' -> ' = string.Empty,'

': "",' -> ': string.Empty,'

'@""\)' -> 'string.Empty)'

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:
  • 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
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT), Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Dongbo Wang (daxian-dbw) and Mark Kraus (markekraus) May 28, 2018 14:51
@iSazonov
Ilya (iSazonov) requested review from Bruce Payette (BrucePay), Francisco Gamino (Francisco-Gamino), James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Andrew (anmenaga), Charu Bassi (charub), Dan Travison (dantraMSFT), Mike Richmond (mirichmo) and sergei (vors) as code owners May 28, 2018 14:51
markekraus
Mark Kraus (markekraus) approved these changes May 28, 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 assuming tests pass

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented May 28, 2018

Copy link
Copy Markdown
Collaborator Author

Mark Kraus (@markekraus) Many thanks for fast review!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) force-pushed the cleanup-codefactor-emptystring branch from dca7c3f to 84ea832 Compare May 28, 2018 15:43
@lzybkr

Jason Shirk (lzybkr) commented May 29, 2018

Copy link
Copy Markdown
Contributor

I don't see the point. Why not just disable the warning and skip the code churn?
stknohg 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) commented May 30, 2018

Copy link
Copy Markdown
Collaborator Author

It is readability issue. If we use advanced editor with code highlighting we haven't problems to see "". If we use a simple editor the string.Empty visibility is much better.
Also see my #6949 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

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

Jason Shirk (lzybkr) commented May 30, 2018

Copy link
Copy Markdown
Contributor

I find "" to be more readable, it's shorter and the syntax is very familiar - we don't give all string literals a name for readability.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BrucePay
Bruce Payette (BrucePay) approved these changes Jun 1, 2018
View reviewed changes

@BrucePay Bruce Payette (BrucePay) left a comment

Copy link
Copy Markdown
Collaborator

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.

Overall the results look good. All the changes seem fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/DscSupport/CimDSCParser.cs Outdated

@BrucePay Bruce Payette (BrucePay) Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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.

Could migrate all of the ", " strings into a constant.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 1, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Constant or static?

Sorry, something went wrong.

Uh oh!

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

@BrucePay Bruce Payette (BrucePay) Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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.

Is there any reason it shouldn't be constant?

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 2, 2018

Copy link
Copy Markdown
Collaborator Author

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.

We already have a lot of static variables for such strings.
I found 4487 hits in 581 file for ", ". So static may be useful.

Sorry, something went wrong.

Uh oh!

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

@daxian-dbw Dongbo Wang (daxian-dbw) Jun 8, 2018

Copy link
Copy Markdown
Member

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.

As long as we don't need to change the string, I prefer constant. But that should be in a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...gement.Automation/cimSupport/cmdletization/xml/cmdlets-over-objects.xmlSerializer.autogen.cs Outdated

@BrucePay Bruce Payette (BrucePay) Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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.

If this is an autogen'ed file should you be changing it or the generator?

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 1, 2018

Copy link
Copy Markdown
Collaborator Author

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.

The files is not regenerated and already was changed manually. Seems we should rename the files and remove "autogen" from file names.

Sorry, something went wrong.

Uh oh!

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

@BrucePay Bruce Payette (BrucePay) Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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.

And remove the comments saying "this is an autogen'ed file, don't edit"

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 2, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Tracking issue #6974

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Jun 6, 2018

Copy link
Copy Markdown
Collaborator Author

Steve Lee (@SteveL-MSFT) Can we merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Travis Plunk (TravisEz13) Jun 6, 2018
@dantraMSFT

Dan Travison (dantraMSFT) commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

Ilya (@iSazonov) What does indicate as the reasoning for this change? Is this a readability or programmatic warning?

My understanding of "" is that it is interned and you will have one instance per assembly. In all other regards, String.Empty is synonymous with "".
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jun 8, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/Modules/ScriptAnalysis.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jun 8, 2018

Copy link
Copy Markdown
Member

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.

This should be reverted.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Good catch!
Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jun 8, 2018
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/ConfigProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jun 8, 2018

Copy link
Copy Markdown
Member

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.

missing replacing an instance of ""

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jun 8, 2018
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/ConfigProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jun 8, 2018

Copy link
Copy Markdown
Member

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.

Here too.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jun 8, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Dan Travison (@dantraMSFT) I think the purpose is to enable the style checking about empty string (favor string.Emtpy over ""). That rule cannot be turned on today due to too much noise.
The intern string should not be a concern, now it's one instance per assembly, but after replacing with string.Empty it will be one instance per process.

Ilya (@iSazonov) Thanks for spending efforts on driving the style consistency. I ran check-stringreplacementchange-ps1 with this PR and the results look good except for the three comments I left.

$log = .\Check-StringReplacementChange.ps1 -PRUrl https://github.com/PowerShell/PowerShell/pull/6950 -OriginalText '@""','""' -NewText 'string.Empty' -PassThru

I quickly go through the changes and didn't notice any replacements in PowerShell scripts in .cs files, that's good because string.Emtpy will definitely fail in powershell script. However, since I went through so quickly, I could miss something, so please double check that you are not replacing anything in powershell script (script strings). After double checking that, the PR should be good.
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

Dan Travison (@dantraMSFT) The PR has a purpose to turn CodeFactor into a more useful tool. Currently we see over 100000 issues most of which are style issues.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) force-pushed the cleanup-codefactor-emptystring branch from 18ed825 to 3ef9ef0 Compare June 9, 2018 04:26
Ilya (iSazonov) added 18 commits June 9, 2018 10:08
@iSazonov
Replace '= "";' -> '= string.Empty;'
c08a96b
@iSazonov
Replace 'return "";' -> 'return string.Empty;'
faa2257
@iSazonov
Replace ': "";' -> ': string.Empty;'
6ca480a
@iSazonov
Replace '\?\? "";' -> '?? string.Empty;'
8871dd5
@iSazonov
Replace '\(""\)' -> '(string.Empty)'
72e7436
@iSazonov
Replace ' ""\)' -> ' string.Empty)'
fbd2e6d
@iSazonov
Replace '\("",' -> '(string.Empty,'
b0f086a
@iSazonov
Replace ', "",' -> ', string.Empty,'
0827f2c
@iSazonov
Replace ',""\)' -> ', string.Empty)'
af1a0a0
@iSazonov
Replace '\? "" :' -> '? string.Empty :'
225813d
@iSazonov
Replace '\?\? ""' -> '?? string.Empty'
909b240
@iSazonov
Replace ' = "",' -> ' = string.Empty,'
6721052
@iSazonov
Replace ': "",' -> ': string.Empty,'
efe051f
@iSazonov
Replace '@""\)' -> 'string.Empty)'
045cbc0
@iSazonov
Manual change 1
8c66be7
@iSazonov
Manual change 2
8b7e743
@iSazonov
[Feature] Fix defaults
acb7971
@iSazonov
Address feedback
dec9f0c
@iSazonov
Ilya (iSazonov) force-pushed the cleanup-codefactor-emptystring branch from 3ef9ef0 to dec9f0c Compare June 9, 2018 05:12
@iSazonov

Ilya (iSazonov) commented Jun 9, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@daxian-dbw) I re-check - no scripts was changed.

Rebased to remove PSReadline.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Jun 12, 2018
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit e177fca into PowerShell:master Jun 12, 2018
@iSazonov
Ilya (iSazonov) deleted the cleanup-codefactor-emptystring branch June 13, 2018 03:39
@iSazonov Ilya (iSazonov) mentioned this pull request Nov 28, 2019
Fix regression in Get-PSCallStack #11210
Merged
7 of 14 tasks
xtqqczze added a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Nov 22, 2020
@xtqqczze
Fix SA1122: Use string.Empty for empty strings
9da1bd6 
Follow-up to PowerShell#6950.

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1122.md
@xtqqczze xtqqczze mentioned this pull request Nov 22, 2020
Fix SA1122: Use string.Empty for empty strings #14218
Merged
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @daxian-dbw
CodeFactor code style cleanup: replace literal empty strings with 'st…
10523a7 
…ring.Empty' (PowerShell#6950)
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
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13 TravisEz13 is a code owner
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@charub Charu Bassi (charub) Awaiting requested review from charub
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
@Francisco-Gamino Francisco Gamino (Francisco-Gamino) Awaiting requested review from Francisco-Gamino
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@mirichmo Mike Richmond (mirichmo) Awaiting requested review from mirichmo
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@vors sergei (vors) Awaiting requested review from vors
+2 more reviewers
@BrucePay Bruce Payette (BrucePay) BrucePay approved these changes
@markekraus Mark Kraus (markekraus) markekraus approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@TravisEz13 Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

7 participants

@iSazonov @lzybkr @dantraMSFT @daxian-dbw @markekraus @BrucePay @TravisEz13
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 01402503 — 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.