Cleanup CodeFactor issues with null comparisions #6949

iSazonov · 2018-05-28T12:08:03Z

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 null != var with var != null and null == var with var == null

All changes is made in VS Code with Regex patterns - one pattern by commit:

'\((null) != (\w+)\)' -> '($2 != $1)'

'\((null) != (\w+.\w+)\)' -> '($2 != $1)'

'\((null) != (\w+) ' -> '($2 != $1 '

'(null) != (\w+),' -> '$2 != $1,'

'if \((null) != (\w+(.\w+)+)\)' -> 'if ($2 != $1)'

'if \((null) != (.+)\)' -> 'if ($2 != $1)'

Manual replacement 1

'if \((null) != (\w+.\w+)' -> 'if ($2 != $1'

'\b(null) != (\w+)\)' -> '$2 != $1)'

'\b(null) != (\w+.\w+)\)' -> '$2 != $1)'

'\b(null) != (\w+.\w+.\w+)\)' -> '$2 != $1)'

'\((null) != (\w+.\w+),' -> '($2 != $1,'

'\((null) != (\w+.\w+) ' -> '($2 != $1 '

'\((null) != (\w+.\w+\[\w+.\w+\])' -> '($2 != $1'

'\b(null) != (\w+.\w+)' -> $2 != $1

------------------------------------------

'\((null) == (\w+)\)' -> '($2 == $1)'

'\((null) == (\w+.\w+)\)' -> '($2 == $1)'

'\((null) == (\w+) ' -> '($2 == $1 '

'if \((null) == (\w+.\w+)' -> 'if ($2 == $1'

'\b(null) == (\w+)\)' -> '$2 == $1)'

'\b(null) == (\w+.\w+)\)' -> '$2 == $1)'

'\((null) == (\w+.\w+\["\w+"].\w+)' -> '($2 == $1'

'\((null) == (\w+.\w+\["\w+"])' -> '($2 == $1'

PR Checklist

markekraus · 2018-05-28T12:18:45Z

src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs

this one is out of place. it should go after DeserializingTypeConverter.RehydrationFlags.MissingPropertyOk)

This is single manual change I did :-)
The commit is replaced.

Also removed extra curves

lzybkr · 2018-05-29T16:53:25Z

People use if (null != something) out of habit because some languages like C and C++ allow if (something = null) and that's a nasty bug. I'm not a fan of discouraging this style even though it's not a problem in C#.

If this CodeFactor warning didn't find a bug, I'd disable the warning, it doesn't feel like it's worth the risk of so many changes via regex without much higher confidence the tests cover all of these changes.

iSazonov · 2018-05-30T04:58:05Z

If you remember a year ago, I offered to fix style in tests. Response team was negative - it is better to spend your time on more useful things. It was a convincing position. Then we continued to receive many-many comments on the style of tests each PR. Finally everyone ran out of patience and we fixed our tests.
We have long endured trailing spaces in our code. At one point, our patience ran out and we fixed it.
Now we understand that spend a lot of useless efforts to style and formatting. So @TravisEz13 suggested the use of CodeFactor.
We must configure it before we can effectively use it. This is being discussed in #6930.
We disable obvious rules which we'll never fix like "one type per file" or "ordering elements" and more.
The remaining problems, we have to be correct (partially) to use CodeFactor effectively.
Should we disable current issue? I believe no because we already discussed this and we already require that use the C# style in our code. If we leave bad samples we lose meaning use CodeFactor which reports problems a entire file rather than the latest changes.
About risk see my #6930 (comment)

lzybkr · 2018-05-30T13:19:34Z

I sampled the report from CodeFactor when it was first enabled and wasn't impressed. Maybe after you refine the rules it will show some value, but I'm still skeptical.

As one data point for my skepticism - you touched 187 files in this PR, but CodeFactor claims there are 119 issues fixed and only 99 of the issues fixed are of the Do not use comparison where value goes first.

So if you found 88 or more places to fix that the tool didn't, humans will continue to find similar nits that the tool won't. Contributors will have one additional barrier to contributing in addition to the human nitpickers.

If the nitpicking isn't adding value, you could try asking those making the comments to try less nitpicking.

iSazonov · 2018-05-30T14:01:37Z

When I joined the project, the first three months I was amazed each time I got different types of style comments from different people. Why team did it? Obviously to get good code. What changed this time? Nothing. We will not be able to change our behavior if we do not take any steps.
This situation zugzwang. If we do not correct these issues we are doomed to many style comments. Fix all - it's not real. We need to find a compromise by configuring CodeFactor and fixing some issues.
I would be happy not to do this work. But I definitely envy CoreFX code looks like.

As one data point for my skepticism - you touched 187 files in this PR, but CodeFactor claims there are 119 issues fixed and only 99 of the issues fixed are of the Do not use comparison where value goes first.

On CodeFactor site it is mentioned that they use their config and some other analysis methods - so we can get "weird" and "amazing" results. After we merge our config we can investigate problems if we find them.

lzybkr · 2018-05-30T15:18:25Z

In this specific case - I find it reasonable to adopt the exact opposite of what CodeFactor suggests, but I think it is even more reasonable to disable the warning and move on to more important issues.

Sometimes word order matters, sometimes the order of words doesn't matter. Either way, we understand the message clearly.

If code reviewers are complaining about this exact issue - they too should find more valuable things to complain about.

lzybkr · 2018-05-30T16:23:39Z

My last comment on this specific issue - CoreFx is roughly as inconsistent as PowerShell, with about 6% of the comparisons using null on the LHS.

With some irony, if you fix this, we'd be less like CoreFx. :)

iSazonov · 2018-06-01T08:09:57Z

As one data point for my skepticism - you touched 187 files in this PR, but CodeFactor claims there are 119 issues fixed and only 99 of the issues fixed

I believe that this difference comes from the fact that some code is under if UNIX (and we have still if !CORECLR).

dantraMSFT · 2018-06-08T19:11:59Z

@iSazonov, I have to agree with @lzybkr, I see no benefit to making this change and the rule should simply be disabled.

iSazonov · 2018-06-09T06:22:42Z

I think this rule should remain enabled and this change can be closed if the team does not want it. /cc @SteveL-MSFT @daxian-dbw

SteveL-MSFT · 2018-06-17T21:27:11Z

I scrolled through the changes and don't see any issues with the change itself. So the risk seems low.

As for whether we want to make this change, I think readability is slightly better with null on the RHS. If we want new code to adopt RHS, then I think we should take this PR so we don't have mixed usage. Otherwise, we should just be ok with null on LHS or RHS (although perhaps at least consistent within the same file).

Since @iSazonov has already done the work, risk seems low, and (personally) readability is better. I would be for taking this change. However, I think it's a decision the @PowerShell/powershell-maintainers should make.

iSazonov · 2018-06-18T03:50:55Z

Resolve merge conflicts.

TravisEz13 · 2018-06-20T19:20:09Z

restarted Linux CI, I'll be OOF for a couple of days.

daxian-dbw · 2018-06-21T23:52:01Z

@TravisEz13 @adityapatwardhan and I discussed this offline and we agree to take this PR for the same reasons as @SteveL-MSFT mentioned above. But in general, we prefer less changes per PR for style related changes:

A PR is better to focus on a single category of style issue.
A PR is better to modify less than 10 files (it also depends on how many changes per file)

I will soon go through this PR and merge it.

iSazonov · 2018-06-22T03:58:31Z

•A PR is better to modify less than 10 files (it also depends on how many changes per file)

I suggest to apply this rule only to code. We have tons style issues for doc comments - I think we could change more files per PR in the case without risk.

Seems also the rule assumes that we could fix style issues by file, by module, by cmdlet set (ex., web cmdlets), by provider and so on.

Does this conclusion imply that our policy is changed and we want and accept style changes?
I believe we should update our internal contributing docs.

daxian-dbw · 2018-06-25T18:53:10Z

@iSazonov Doc comments issues can be an exception, because of the super low risk. It's not a rule, but a flexible guideline for us maintainers to consider (you can read the tone -- prefer, better to :)).

The main reason that we don't want massive changes in a PR is the time cost to review -- it will be easy for the PR to end up with no approval since no one wants to spend the time reviewing all the changes. This will, in turn, hurt the author's feeling and discourage the author's further contribution, because if the author really meant to fix those style issues, he/she must have put a lot effort/time into it. So we want to set some guidelines to help prevent a contributor from getting in that situation.

daxian-dbw · 2018-06-25T19:47:58Z

Seems also the rule assumes that we could fix style issues by file, by module, by cmdlet set (ex., web cmdlets), by provider and so on.

Does this conclusion imply that our policy is changed and we want and accept style changes?
I believe we should update our internal contributing docs.

We accept PRs that are purely for style changes (we have to if we want to make CodeFactor useful). Such a PR should be reviewable, and that's why we proposed the flexible guideline. But this should be temporary -- once the existing style issues reported by CodeFactor are fixed, we won't accept PR with pure style changes.

daxian-dbw

Went through the changes. LGTM.

Comment has been addressed.

iSazonov · 2018-06-26T06:29:26Z

@daxian-dbw Thanks for code review and clarifications!

… a comparison expression (#6949)" This reverts commit 4fc784e.

iSazonov added 6 commits May 28, 2018 13:34

Replace $(null) != (\w+)$ -> ($2 != $1)

d44fa0d

Replace $(null) != (\w+.\w+)$ -> ($2 != $1)

0e50d2a

Replace '\((null) != (\w+) ' -> '($2 != $1 '

da71c6f

Replace '(null) != (\w+),' -> '$2 != $1,'

3015b35

Replace 'if $(null) != (\w+(.\w+)+)$' -> 'if ($2 != $1)'

f4805ce

Replace 'if $(null) != (.+)$' -> 'if ($2 != $1)'

cd1b49d

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

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

markekraus previously requested changes May 28, 2018

View reviewed changes

iSazonov added 10 commits May 28, 2018 17:41

Manual replacement 1

6d86343

Replace 'if \((null) != (\w+.\w+)' -> 'if ($2 != $1'

087ab23

Replace '\b(null) != (\w+)\)' -> '$2 != $1)'

2e1f537

Replace \b(null) != (\w+.\w+)\) -> '$2 != $1)'

753348e

Replace '\b(null) != (\w+.\w+.\w+)\)' -> '$2 != $1)'

a801dde

Replace '\((null) != (\w+.\w+),' -> '($2 != $1,'

d57088d

Replace '\((null) != (\w+.\w+) ' -> '($2 != $1 '

1487874

Also removed extra curves

Replace '\((null) != (\w+.\w+\[\w+.\w+\])' -> '($2 != $1'

757fcf7

Replace '\b(null) != (\w+.\w+)' -> $2 != $1

93890ad

Replace '$(null) == (\w+)$' -> '($2 == $1)'

658c33b

iSazonov mentioned this pull request May 30, 2018

Cleanup CodeFactor issues with empty strings #6950

Merged

11 tasks

iSazonov mentioned this pull request May 31, 2018

Add Settings.StyleCop file #6930

Merged

11 tasks

bergmeister approved these changes May 31, 2018

View reviewed changes

Merge branch 'master' into cleanup-codefactor-null

8852a33

daxian-dbw self-assigned this Jun 21, 2018

daxian-dbw added 3 commits June 25, 2018 13:07

Remove the awkward wrong code.

e9cbab6

Remove the wrong code.

7506a03

Update one more similar case.

052c215

daxian-dbw approved these changes Jun 25, 2018

View reviewed changes

daxian-dbw merged commit 4fc784e into PowerShell:master Jun 25, 2018

iSazonov deleted the cleanup-codefactor-null branch June 26, 2018 06:27

daxian-dbw added a commit that referenced this pull request Jun 26, 2018

Revert "Bulk update code base to put 'null' on the right-hand-side of…

f892b7c

… a comparison expression (#6949)" This reverts commit 4fc784e.

Cleanup CodeFactor issues with null comparisions #6949

Cleanup CodeFactor issues with null comparisions #6949

Uh oh!

Conversation

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!

markekraus May 28, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov May 28, 2018

Choose a reason for hiding this comment

Uh oh!

lzybkr commented May 29, 2018

Uh oh!

iSazonov commented May 30, 2018

Uh oh!

lzybkr commented May 30, 2018

Uh oh!

iSazonov commented May 30, 2018

Uh oh!

lzybkr commented May 30, 2018

Uh oh!

lzybkr commented May 30, 2018

Uh oh!

iSazonov commented Jun 1, 2018

Uh oh!

dantraMSFT commented Jun 8, 2018

Uh oh!

iSazonov commented Jun 9, 2018

Uh oh!

SteveL-MSFT commented Jun 17, 2018

Uh oh!

iSazonov commented Jun 18, 2018

Uh oh!

TravisEz13 commented Jun 20, 2018

Uh oh!

daxian-dbw commented Jun 21, 2018

Uh oh!

iSazonov commented Jun 22, 2018

Uh oh!

daxian-dbw commented Jun 25, 2018

Uh oh!

daxian-dbw commented Jun 25, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

iSazonov commented May 28, 2018 •

edited

Loading