Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Enable IDE0048: AddRequiredParentheses#13936

Closed
xtqqczze wants to merge 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:IDE0048-p2xtqqczze/PowerShell-PowerShell:IDE0048-p2Copy head branch name to clipboard
Closed

Enable IDE0048: AddRequiredParentheses#13936
xtqqczze wants to merge 3 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
xtqqczze:IDE0048-p2xtqqczze/PowerShell-PowerShell:IDE0048-p2Copy head branch name to clipboard

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Oct 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@ghost ghost assigned anmenaga Oct 29, 2020
@xtqqczze xtqqczze mentioned this pull request Oct 29, 2020
Merged
@xtqqczze xtqqczze marked this pull request as ready for review October 30, 2020 01:13
@xtqqczze xtqqczze mentioned this pull request Oct 31, 2020
Merged
@xtqqczze xtqqczze changed the title Apply IDE0048: AddRequiredParentheses WIP: Enable IDE0048: AddRequiredParentheses p2 Oct 31, 2020
@xtqqczze xtqqczze marked this pull request as draft October 31, 2020 17:47
@xtqqczze xtqqczze changed the title WIP: Enable IDE0048: AddRequiredParentheses p2 Enable IDE0048: AddRequiredParentheses p2 Nov 1, 2020
@xtqqczze xtqqczze marked this pull request as ready for review November 1, 2020 19:51
@xtqqczze xtqqczze changed the title Enable IDE0048: AddRequiredParentheses p2 Enable IDE0048: AddRequiredParentheses Nov 2, 2020
iSazonov
iSazonov reviewed Nov 2, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/hostifaces/LocalConnection.cs Outdated
iSazonov
iSazonov reviewed Nov 2, 2020
View reviewed changes

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xtqqczze

xtqqczze commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

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

iSazonov
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

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less readability.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

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.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less readability.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

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

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less readability.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

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

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less readability.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/Set-PSBreakpoint.cs Outdated
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs Outdated
Comment on lines 2009 to 2010

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less readability.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

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

@iSazonov iSazonov Nov 2, 2020

Copy link
Copy Markdown
Collaborator

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.

@xtqqczze xtqqczze Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiplicative > Additive

Comment thread src/System.Management.Automation/FormatAndOutput/common/OutputQueue.cs Outdated
@xtqqczze

xtqqczze commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

@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 *, /, %, +, -, <<, >>, &, ^, |

@iSazonov

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.

@xtqqczze

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?

@iSazonov

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.

@iSazonov

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.

@iSazonov iSazonov closed this Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

Assignees

@anmenaga anmenaga

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xtqqczze @iSazonov @anmenaga
Morty Proxy This is a proxified and sanitized view of the page, visit original site.