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

Ilya (iSazonov) · 2018-05-28T14:51:22Z

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

Mark Kraus (markekraus)

LGTM assuming tests pass

Ilya (iSazonov) · 2018-05-28T15:10:51Z

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

Jason Shirk (lzybkr) · 2018-05-29T16:55:12Z

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

Ilya (iSazonov) · 2018-05-30T05:09:38Z

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)

Jason Shirk (lzybkr) · 2018-05-30T13:22:51Z

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.

Bruce Payette (BrucePay)

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

Bruce Payette (BrucePay) · 2018-06-01T06:16:03Z

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.

Bruce Payette (BrucePay) · 2018-06-01T06:20:44Z

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

Ilya (iSazonov) · 2018-06-06T05:02:11Z

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

Dan Travison (dantraMSFT) · 2018-06-08T19:22:35Z

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

Dongbo Wang (daxian-dbw) · 2018-06-08T21:26:02Z

This should be reverted.

Good catch!
Fixed.

Dongbo Wang (daxian-dbw) · 2018-06-08T21:27:08Z

missing replacing an instance of ""

Dongbo Wang (daxian-dbw) · 2018-06-08T21:27:20Z

Dongbo Wang (daxian-dbw) · 2018-06-08T22:35:47Z

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.

Ilya (iSazonov) · 2018-06-09T04:08:55Z

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.

Ilya (iSazonov) · 2018-06-09T05:25:15Z

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)

Uh oh!

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

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