Cleanup CodeFactor issues with empty strings by iSazonov · Pull Request #6950 · PowerShell/PowerShell · GitHub

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

LGTM assuming tests pass

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

I don't see the point. Why not just disable the warning and skip the code churn?

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)

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.

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

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

Constant or static?

Is there any reason it shouldn't be constant?

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

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

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

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

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

Tracking issue #6974

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

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 "".

This should be reverted.

Good catch!
Fixed.

missing replacing an instance of ""

Fixed.

Here too.

Fixed.

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.

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.

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

Rebased to remove PSReadline.

Follow-up to PowerShell#6950. https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1122.md

…ring.Empty' (PowerShell#6950)

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

Mark Kraus (markekraus) approved these changes May 28, 2018

View reviewed changes

Ilya (iSazonov) force-pushed the cleanup-codefactor-emptystring branch from dca7c3f to 84ea832 Compare May 28, 2018 15:43

Bruce Payette (BrucePay) approved these changes Jun 1, 2018

View reviewed changes

Steve Lee (SteveL-MSFT) assigned Travis Plunk (TravisEz13) Jun 6, 2018

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

View reviewed changes

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

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

c08a96b

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

faa2257

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

6ca480a

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

8871dd5

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

72e7436

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

fbd2e6d

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

b0f086a

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

0827f2c

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

af1a0a0

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

225813d

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

909b240

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

6721052

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

efe051f

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

045cbc0

Manual change 1

8c66be7

Manual change 2

8b7e743

[Feature] Fix defaults

acb7971

Address feedback

dec9f0c

Ilya (iSazonov) force-pushed the cleanup-codefactor-emptystring branch from 3ef9ef0 to dec9f0c Compare June 9, 2018 05:12

Dongbo Wang (daxian-dbw) approved these changes Jun 12, 2018

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit e177fca into PowerShell:master Jun 12, 2018

Ilya (iSazonov) deleted the cleanup-codefactor-emptystring branch June 13, 2018 03:39

Ilya (iSazonov) mentioned this pull request Nov 28, 2019

Fix regression in Get-PSCallStack #11210

Merged

14 tasks

xtqqczze added a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Nov 22, 2020

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 mentioned this pull request Nov 22, 2020

Fix SA1122: Use string.Empty for empty strings #14218

Merged

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

CodeFactor code style cleanup: replace literal empty strings with 'st…

10523a7

…ring.Empty' (PowerShell#6950)

Cleanup CodeFactor issues with empty strings by iSazonov · Pull Request #6950 · PowerShell/PowerShell · GitHub

Uh oh!

Cleanup CodeFactor issues with empty strings - #6950

Cleanup CodeFactor issues with empty strings#6950Dongbo Wang (daxian-dbw) merged 18 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom iSazonov:cleanup-codefactor-emptystringiSazonov/PowerShell:cleanup-codefactor-emptystringCopy head branch name to clipboard

Conversation

Ilya (iSazonov) commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Mark Kraus (markekraus) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented May 28, 2018

Uh oh!

Jason Shirk (lzybkr) commented May 29, 2018

Uh oh!

Ilya (iSazonov) commented May 30, 2018

Uh oh!

Jason Shirk (lzybkr) commented May 30, 2018

Uh oh!

Bruce Payette (BrucePay) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Jun 6, 2018

Uh oh!

Dan Travison (dantraMSFT) commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jun 9, 2018

Uh oh!

Ilya (iSazonov) commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Cleanup CodeFactor issues with empty strings#6950
Dongbo Wang (daxian-dbw) merged 18 commits into
PowerShell:masterfrom
iSazonov:cleanup-codefactor-emptystring

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

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

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