-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix lgtm issues #8843
Fix lgtm issues #8843
Conversation
It seems like this PR is making the Code factor grade decline. |
@RDIL No, the issues already was there. CodeFactor has strange reports sometimes. |
Ah okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web cmdlet changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with non-blocking suggestions
@@ -1451,7 +1451,7 @@ public class DriveMatchingCoreCommandBase : CoreCommandBase | |||
} | |||
else | |||
{ | |||
if (nameMatcher.IsMatch(drive.Name)) | |||
if (nameMatcher != null && nameMatcher.IsMatch(drive.Name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be
if (nameMatcher != null && nameMatcher.IsMatch(drive.Name)) | |
if (nameMatcher?.IsMatch(drive.Name) ?? false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Report is "Cannot implicitly convert type bool? to bool."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this works?
if (nameMatcher?.IsMatch(drive.Name) ?? false)
{
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks as puzzle :-)
@@ -8071,8 +8071,10 @@ private static string WinInternalGetTarget(SafeFileHandle handle) | |||
targetDir = Encoding.Unicode.GetString(reparseDataBufferMountPoint.PathBuffer, reparseDataBufferMountPoint.SubstituteNameOffset, reparseDataBufferMountPoint.SubstituteNameLength); | |||
} | |||
|
|||
if (targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) | |||
if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be
if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) | |
if (targetDir?.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase) ?? false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the cleanup! 👍 😍
SetRequestContent(request, form.Fields); | ||
} | ||
else if (content is IDictionary && request.Method != HttpMethod.Get) | ||
else if (content is IDictionary dictionary && request.Method != HttpMethod.Get) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of if
/else if
going on here. Can this be refactored into a pattern-matched switch instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purpose of this PR is to fix null checks. So code refactoring is out of the PR. Feel free to push new PR if you see benefits.
{ | ||
cellCount = _lo.DisplayCells.Length(fpf.propertyValue); | ||
if (widths[kk] < cellCount) | ||
widths[kk] = cellCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've seen of the codebase, it seems like the inline / two-line if
statement is generally avoided in favor of the explicitly-braced form, even for simple if statements. Should this follow the existing patterns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
{ | ||
cellCount = _lo.DisplayCells.Length(fpf.propertyValue); | ||
if (cellCount > maxLen) | ||
maxLen = cellCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vexx32 I consider such patterns as a "security" risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate @iSazonov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
PR Context
Come from https://lgtm.com/projects/g/PowerShell/PowerShell/alerts/?mode=tree&ruleFocus=1506101336231
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests