Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Cleanup CodeFactor issues with empty strings#6950

Merged
daxian-dbw merged 18 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:cleanup-codefactor-emptystringiSazonov/PowerShell:cleanup-codefactor-emptystringCopy head branch name to clipboard
Jun 12, 2018
Merged

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

Conversation

@iSazonov

@iSazonov iSazonov commented May 28, 2018 •
edited
Loading

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

markekraus
markekraus approved these changes May 28, 2018
View reviewed changes

@markekraus markekraus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming tests pass

@iSazonov

iSazonov commented May 28, 2018

Copy link
Copy Markdown
Collaborator Author

@markekraus Many thanks for fast review!

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

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?

@iSazonov

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)

@lzybkr

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.

BrucePay
BrucePay approved these changes Jun 1, 2018
View reviewed changes

@BrucePay BrucePay left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/System.Management.Automation/DscSupport/CimDSCParser.cs Outdated

@BrucePay BrucePay Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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.

@iSazonov iSazonov Jun 1, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant or static?

@BrucePay BrucePay Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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?

@iSazonov iSazonov Jun 2, 2018

Copy link
Copy Markdown
Collaborator Author

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.

@daxian-dbw daxian-dbw Jun 8, 2018

Copy link
Copy Markdown
Member

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.

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

@BrucePay BrucePay Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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?

@iSazonov iSazonov Jun 1, 2018

Copy link
Copy Markdown
Collaborator Author

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.

@BrucePay BrucePay Jun 1, 2018

Copy link
Copy Markdown
Collaborator

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"

@iSazonov iSazonov Jun 2, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking issue #6974

@iSazonov

iSazonov commented Jun 6, 2018

Copy link
Copy Markdown
Collaborator Author

@SteveL-MSFT Can we merge?

@dantraMSFT

dantraMSFT commented Jun 8, 2018

Copy link
Copy Markdown
Contributor

@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
daxian-dbw reviewed Jun 8, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/Modules/ScriptAnalysis.cs Outdated

@daxian-dbw daxian-dbw Jun 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

@iSazonov iSazonov Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
Fixed.

daxian-dbw
daxian-dbw reviewed Jun 8, 2018
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/ConfigProvider.cs Outdated

@daxian-dbw daxian-dbw Jun 8, 2018

Copy link
Copy Markdown
Member

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

@iSazonov iSazonov Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

daxian-dbw
daxian-dbw reviewed Jun 8, 2018
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/ConfigProvider.cs Outdated

@daxian-dbw daxian-dbw Jun 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

@iSazonov iSazonov Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@daxian-dbw

daxian-dbw commented Jun 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@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

iSazonov commented Jun 9, 2018

Copy link
Copy Markdown
Collaborator Author

@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 iSazonov force-pushed the cleanup-codefactor-emptystring branch from 18ed825 to 3ef9ef0 Compare June 9, 2018 04:26
@iSazonov iSazonov force-pushed the cleanup-codefactor-emptystring branch from 3ef9ef0 to dec9f0c Compare June 9, 2018 05:12
@iSazonov

iSazonov commented Jun 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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

Rebased to remove PSReadline.

@daxian-dbw daxian-dbw merged commit e177fca into PowerShell:master Jun 12, 2018
@iSazonov iSazonov deleted the cleanup-codefactor-emptystring branch June 13, 2018 03:39
@iSazonov iSazonov mentioned this pull request Nov 28, 2019
Merged
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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga

@charub charub Awaiting requested review from charub

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@mirichmo mirichmo Awaiting requested review from mirichmo

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@vors vors Awaiting requested review from vors

+2 more reviewers

@BrucePay BrucePay BrucePay approved these changes

@markekraus markekraus markekraus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@TravisEz13 TravisEz13

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@iSazonov @lzybkr @dantraMSFT @daxian-dbw @markekraus @BrucePay @TravisEz13
Morty Proxy This is a proxified and sanitized view of the page, visit original site.