GeezerVison

The rule is contradictory: parentheses are good for complex expressions to eliminate errors, but no doubt they are superfluous for simple expressions where they worsen readability.

I think it is better to set "suggestion" for the rule.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze commented Nov 2, 2020

Copy link

Copy Markdown

Contributor Author

Ilya (@iSazonov) Could you leave more review comments to give examples of where readability has been worsened?

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) reviewed

Nov 2, 2020

View reviewed changes

Comment thread

src/Microsoft.PowerShell.Commands.Management/commands/management/GetComputerInfoCommand.cs Outdated

Comment on lines 1162 to 1165

Ilya (iSazonov) Nov 2, 2020

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.

Less readability.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

For most people it is obvious that multiplication has a higher precedence than addition, so the parenthesis are probably not necessary. However while readability might not be any better, it is not really any worse.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

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/TestConnectionCommand.cs Outdated

Ilya (iSazonov) Nov 2, 2020

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.

Less readability.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs Outdated

Ilya (iSazonov) Nov 2, 2020

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.

Less readability.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs Outdated

Ilya (iSazonov) Nov 2, 2020

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.

Less readability.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Set-PSBreakpoint.cs Resolved

Uh oh!

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

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs Outdated

Comment on lines 2009 to 2010

Ilya (iSazonov) Nov 2, 2020

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.

Less readability.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs Outdated

Ilya (iSazonov) Nov 2, 2020

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.

I'd wonder if anybody wanted parentheses in such expressions.

Sorry, something went wrong.

Uh oh!

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

All reactions

xtqqczze Nov 2, 2020

Copy link

Copy Markdown

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

Multiplicative > Additive

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/OutputQueue.cs Resolved

Uh oh!

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

xtqqczze commented Nov 2, 2020

Copy link

Copy Markdown

Contributor Author

Ilya (@iSazonov) It seems all your examples of worsened readability are for the higher precedence of multiplicative vs additive operators.

Unfortunately the analyzer does not provide configuration at this level, the closest option is dotnet_style_parentheses_in_arithmetic_binary_operators covering *, /, %, +, -, <<, >>, &, ^, |

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 2, 2020

Copy link

Copy Markdown

Collaborator

This is a controversial rule that depends on context. For simple expressions, no additional parentheses are needed. For complex expressions, good formatting and descriptive names are more important. I suppose we can turn this off at all.

All reactions

Sorry, something went wrong.

Uh oh!

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

xtqqczze commented Nov 2, 2020

Copy link

Copy Markdown

Contributor Author

I suppose we can turn this off at all.

OK so we use a lesser severity, suggestion or hidden?

All reactions

Sorry, something went wrong.

Uh oh!

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

xtqqczze added 3 commits

November 2, 2020 10:52


          Enable IDE0048: AddRequiredParentheses

a779b1c

https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0047-ide0048


          fixup! Enable IDE0048: AddRequiredParentheses

a3f49ea


          Reduce severity to suggestion

c3dd439

xtqqczze force-pushed the IDE0048-p2 branch from 1a6853d to c3dd439 Compare

November 2, 2020 10:55

Ilya (iSazonov) commented Nov 2, 2020

Copy link

Copy Markdown

Collaborator

OK so we use a lesser severity, suggestion or hidden?

Is there Quick fix in VS Code? If yes, I agree with "suggestion", otherwise it is better to hide.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 5, 2020

Copy link

Copy Markdown

Collaborator

xtqqczze I close the PR since it is controversial. I suggest these rules to "suggestion" in new PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) closed this

Nov 5, 2020

Reviewers

Ilya (iSazonov)

iSazonov left review comments

Aditya Patwardhan (adityapatwardhan)

Awaiting requested review from adityapatwardhan

Andrew (anmenaga)

Awaiting requested review from anmenaga

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

daxian-dbw is a code owner

Steve Lee (SteveL-MSFT)

Awaiting requested review from SteveL-MSFT

Travis Plunk (TravisEz13)

Awaiting requested review from TravisEz13

TravisEz13 is a code owner

Uh oh!

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

Assignees

Andrew (anmenaga)

Labels

None yet

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.

3 participants

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.

Conversation

xtqqczze commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

xtqqczze commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xtqqczze commented Nov 2, 2020

Uh oh!

Ilya (iSazonov) commented Nov 2, 2020

Uh oh!

xtqqczze commented Nov 2, 2020

Uh oh!

Ilya (iSazonov) commented Nov 2, 2020

Uh oh!

Ilya (iSazonov) commented Nov 5, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xtqqczze commented Oct 29, 2020 •

edited

Loading

Uh oh!

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