Cleanup CodeFactor issues with empty strings #6950

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

markekraus

LGTM assuming tests pass

iSazonov · 2018-05-28T15:10:51Z

@markekraus Many thanks for fast review!

lzybkr · 2018-05-29T16:55:12Z

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

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)

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.

BrucePay

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

BrucePay · 2018-06-01T06:16:03Z

src/System.Management.Automation/DscSupport/CimDSCParser.cs

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.

BrucePay · 2018-06-01T06:20:44Z

...gement.Automation/cimSupport/cmdletization/xml/cmdlets-over-objects.xmlSerializer.autogen.cs

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

iSazonov · 2018-06-06T05:02:11Z

@SteveL-MSFT Can we merge?

dantraMSFT · 2018-06-08T19:22:35Z

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

daxian-dbw · 2018-06-08T21:26:02Z

src/System.Management.Automation/engine/Modules/ScriptAnalysis.cs

This should be reverted.

Good catch!
Fixed.

daxian-dbw · 2018-06-08T21:27:08Z

src/Microsoft.WSMan.Management/ConfigProvider.cs

missing replacing an instance of ""

daxian-dbw · 2018-06-08T21:27:20Z

src/Microsoft.WSMan.Management/ConfigProvider.cs

daxian-dbw · 2018-06-08T22:35:47Z

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

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

iSazonov · 2018-06-09T04:08:55Z

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

iSazonov · 2018-06-09T05:25:15Z

@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

iSazonov requested review from SteveL-MSFT, TravisEz13, adityapatwardhan, daxian-dbw and markekraus May 28, 2018 14:51

iSazonov requested review from BrucePay, Francisco-Gamino, JamesWTruher, PaulHigin, anmenaga, charub, dantraMSFT, mirichmo and vors as code owners May 28, 2018 14:51

markekraus approved these changes May 28, 2018

View reviewed changes

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

BrucePay approved these changes Jun 1, 2018

View reviewed changes

SteveL-MSFT assigned TravisEz13 Jun 6, 2018

daxian-dbw reviewed Jun 8, 2018

View reviewed changes

iSazonov force-pushed the cleanup-codefactor-emptystring branch from 18ed825 to 3ef9ef0 Compare June 9, 2018 04:26

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

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

daxian-dbw approved these changes Jun 12, 2018

View reviewed changes

daxian-dbw merged commit e177fca into C7D9 PowerShell:master Jun 12, 2018

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

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

Cleanup CodeFactor issues with empty strings #6950

Cleanup CodeFactor issues with empty strings #6950

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants