Cleanup CodeFactor issues with empty strings

[iSazonov]

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

[markekraus]

LGTM assuming tests pass

[iSazonov]

@markekraus Many thanks for fast review!

[lzybkr]

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

[iSazonov]

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]

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]

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

[iSazonov]

Constant or static?

[BrucePay]

Is there any reason it shouldn't be constant?

[iSazonov]

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

[daxian-dbw]

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

[BrucePay]

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

[iSazonov]

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

[BrucePay]

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

[iSazonov]

Tracking issue #6974

[iSazonov]

@SteveL-MSFT Can we merge?

[dantraMSFT]

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

This should be reverted.

[iSazonov]

Good catch!
Fixed.

[daxian-dbw]

missing replacing an instance of ""

[iSazonov]

Fixed.

[daxian-dbw]

Here too.

[iSazonov]

Fixed.

[daxian-dbw]

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

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

@daxian-dbw I re-check - no scripts was changed.

Rebased to remove PSReadline.