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. |
markekraus
left a comment
There was a problem hiding this comment.
Web cmdlet changes LGTM
adityapatwardhan
left a comment
There was a problem hiding this comment.
Approved with non-blocking suggestions
| else | ||
| { | ||
| if (nameMatcher.IsMatch(drive.Name)) | ||
| if (nameMatcher != null && nameMatcher.IsMatch(drive.Name)) |
There was a problem hiding this comment.
Can this be
| if (nameMatcher != null && nameMatcher.IsMatch(drive.Name)) | |
| if (nameMatcher?.IsMatch(drive.Name) ?? false) |
There was a problem hiding this comment.
Report is "Cannot implicitly convert type bool? to bool."
There was a problem hiding this comment.
Maybe this works?
if (nameMatcher?.IsMatch(drive.Name) ?? false)
{
}There was a problem hiding this comment.
Looks as puzzle :-)
| } | ||
|
|
||
| if (targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) | ||
| if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Can this be
| if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase)) | |
| if (targetDir?.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase) ?? false) |
| 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.
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.
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.
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?
| { | ||
| cellCount = _lo.DisplayCells.Length(fpf.propertyValue); | ||
| if (cellCount > maxLen) | ||
| maxLen = cellCount; |
PR Summary
PR Context
Come from https://lgtm.com/projects/g/PowerShell/PowerShell/alerts/?mode=tree&ruleFocus=1506101336231
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile 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