Skip to content

Enable IDE0048: AddRequiredParentheses#13936

Closed
xtqqczze wants to merge 3 commits intoPowerShell:masterfrom
xtqqczze:IDE0048-p2
Closed

Enable IDE0048: AddRequiredParentheses#13936
xtqqczze wants to merge 3 commits intoPowerShell:masterfrom
xtqqczze:IDE0048-p2

Conversation

@xtqqczze
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze commented Oct 29, 2020 •
edited
Loading

@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
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov left a comment

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
Copy link
Copy Markdown
Contributor Author

xtqqczze commented Nov 2, 2020

@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
src/Microsoft.PowerShell.Commands.Management/commands/management/GetComputerInfoCommand.cs Outdated
Comment on lines 1162 to 1165
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

Choose a reason for hiding this comment

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

Less readability.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

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.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

Choose a reason for hiding this comment

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

Less readability.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs Outdated
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

Choose a reason for hiding this comment

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

Less readability.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs Outdated
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

Choose a reason for hiding this comment

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

Less readability.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs Outdated
Comment on lines 2009 to 2010
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

Choose a reason for hiding this comment

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

Less readability.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs Outdated
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Nov 2, 2020

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.

Copy link
Copy Markdown
Contributor Author

@xtqqczze xtqqczze Nov 2, 2020

Choose a reason for hiding this comment

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

Multiplicative > Additive

@xtqqczze
Copy link
Copy Markdown
Contributor Author

xtqqczze commented Nov 2, 2020

@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
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 2, 2020

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
Copy link
Copy Markdown
Contributor Author

xtqqczze commented Nov 2, 2020

I suppose we can turn this off at all.

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

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 2, 2020

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
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 5, 2020

@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