GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix lgtm issues by iSazonov · Pull Request #8843 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Feb 7, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

  • Add some null checks.
  • Remove unused code.

PR Context

Come from https://lgtm.com/projects/g/PowerShell/PowerShell/alerts/?mode=tree&ruleFocus=1506101336231

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: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A 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] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ilya (iSazonov) added 15 commits February 7, 2019 10:28
@iSazonov
Fix LogMessage()
6abd862
@iSazonov
Remove unused fixedUpAssemblyPathList
3136a83
@iSazonov
Fix ForwardHelp()
64ceb32
@iSazonov
Fix convert-HTML.cs
6e1cb74
@iSazonov
Fix typeDataQuery.cs
2f97a9b
@iSazonov
Fix ScriptWriter.cs
60b6725
@iSazonov
Fix possible null in Navigation.cs
0629bd3
@iSazonov
Add null check in FileSystemProvider
5783f7d
@iSazonov
Add null check in CodeAdapter.cs
d612c3e
@iSazonov
Add null check in PSRemotingCmdlet.cs
0dc2524
@iSazonov
Add null check to InitialSessionState.cs
7eea026
@iSazonov
Fix IndexOf in debugger.cs
a368acf
@iSazonov
Minor refactor in BaseOutputtingCommand.cs
6235c20
@iSazonov
Minor refactor in DefaultCommandHelpObjectBuilder.cs
9ef331b
@iSazonov
Refactor in WebRequestPSCmdlet.Common.cs
e99015f
@iSazonov Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Feb 7, 2019
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT), Travis Plunk (TravisEz13) and Aditya Patwardhan (adityapatwardhan) February 7, 2019 11:24
@iSazonov
Ilya (iSazonov) requested review from Bruce Payette (BrucePay), Francisco Gamino (Francisco-Gamino), James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Andrew (anmenaga), Dongbo Wang (daxian-dbw) and Mike Richmond (mirichmo) as code owners February 7, 2019 11:24
@iSazonov
[Feature]
33ace7e
@RDIL

Reece Dunham (RDIL) commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

It seems like this PR is making the Code factor grade decline.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Feb 7, 2019

Copy link
Copy Markdown
Collaborator Author

Reece Dunham (@RDIL) No, the issues already was there. CodeFactor has strange reports sometimes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RDIL

Reece Dunham (RDIL) commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

Ah okay.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markekraus
Mark Kraus (markekraus) approved these changes Feb 7, 2019
View reviewed changes

@markekraus Mark Kraus (markekraus) left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov and RDIL reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Feb 7, 2019
View reviewed changes

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
else
{
if (nameMatcher.IsMatch(drive.Name))
if (nameMatcher != null && nameMatcher.IsMatch(drive.Name))

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Feb 7, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Suggested change
if (nameMatcher != null && nameMatcher.IsMatch(drive.Name))
if (nameMatcher?.IsMatch(drive.Name) ?? false)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Feb 8, 2019

Copy link
Copy Markdown
Member

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)
{
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs
}

if (targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase))
if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase))

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Feb 7, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Suggested change
if (targetDir != null && targetDir.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase))
if (targetDir?.StartsWith(NonInterpretedPathPrefix, StringComparison.OrdinalIgnoreCase) ?? false)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Feb 8, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vexx32
Rain Sallow (/u/ta11ow) (vexx32) reviewed Feb 7, 2019
View reviewed changes

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) left a comment

Copy link
Copy Markdown
Collaborator

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! 👍 😍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
SetRequestContent(request, form.Fields);
}
else if (content is IDictionary && request.Method != HttpMethod.Get)
else if (content is IDictionary dictionary && request.Method != HttpMethod.Get)

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Feb 7, 2019

Copy link
Copy Markdown
Collaborator

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs
{
cellCount = _lo.DisplayCells.Length(fpf.propertyValue);
if (widths[kk] < cellCount)
widths[kk] = cellCount;

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Feb 7, 2019

Copy link
Copy Markdown
Collaborator

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/FormatAndOutput/common/BaseOutputtingCommand.cs
{
cellCount = _lo.DisplayCells.Length(fpf.propertyValue);
if (cellCount > maxLen)
maxLen = cellCount;

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Feb 7, 2019

Copy link
Copy Markdown
Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Rain Sallow (/u/ta11ow) (@vexx32) I consider such patterns as a "security" risk.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Feb 8, 2019

Copy link
Copy Markdown
Collaborator

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 Ilya (@iSazonov)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Feb 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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.

Famous https://blog.codecentric.de/en/2014/02/curly-braces/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji vexx32 reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
RDIL
Reece Dunham (RDIL) approved these changes Feb 7, 2019
View reviewed changes
Ilya (iSazonov) added 2 commits February 8, 2019 08:50
@iSazonov
Merge branch 'master' into fix-lgtm-issues
4b164b9
@iSazonov
Resolve merge conflict
d155bd2
@iSazonov Ilya (iSazonov) self-assigned this Feb 13, 2019
@iSazonov
Ilya (iSazonov) merged commit ff3edaa into PowerShell:master Feb 13, 2019
@iSazonov
Ilya (iSazonov) deleted the fix-lgtm-issues branch February 13, 2019 11:51
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov
Fix lgtm issues (PowerShell#8843)
4f0b05e 
Add some null checks
Remove unused code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13 TravisEz13 is a code owner
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@Francisco-Gamino Francisco Gamino (Francisco-Gamino) Awaiting requested review from Francisco-Gamino
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@mirichmo Mike Richmond (mirichmo) Awaiting requested review from mirichmo
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
+3 more reviewers
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) vexx32 left review comments
@markekraus Mark Kraus (markekraus) markekraus approved these changes
@RDIL Reece Dunham (RDIL) RDIL approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@iSazonov @RDIL @markekraus @adityapatwardhan @vexx32
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.