Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rule to prefer conditional expression over assignement #1977

Merged
merged 7 commits into from Jul 30, 2019

Conversation

@yahiheb
Copy link
Collaborator

commented Jul 21, 2019

We can use #pragma warning disable IDE0045 // Convert to conditional expression if the conditional expression is not clear.

@nopara73
Copy link
Collaborator

left a comment

Concept NACK. Readability is the most important aspect here and that's not something a robot can decide.

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 21, 2019

@nopara73 I do agree that readability and code cleanliness is very important. I think the proposed code follows that, and conditional expression is used a lot in the code base, that is why I add that rule.

@MaxHillebrand
Copy link
Collaborator

left a comment

Changes GetTorSocks5EndPoint to TorSocks5EndPoint - not sure if this breaks anything.

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

No it doesn't change that, I simply fixed a conflict with that commit. The change was made here in #1945.

WalletWasabi.Gui/Config.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Config.cs Outdated Show resolved Hide resolved
WalletWasabi.Gui/Models/LoadWalletEntry.cs Outdated Show resolved Hide resolved
@@ -103,6 +103,9 @@ csharp_prefer_braces = true:error
#do not suggest readonly fields
dotnet_style_readonly_field = false:error

#prefer conditional expression over assignement
dotnet_style_prefer_conditional_expression_over_assignment = true:error

This comment has been minimized.

Copy link
@molnard

molnard Jul 26, 2019

Collaborator

I think we should not enforce this. Can result in a very complex and unreadable expression in many cases. IMO it should be a suggestion.

This comment has been minimized.

Copy link
@yahiheb

yahiheb Jul 30, 2019

Author Collaborator

I removed the rule because if we add it even as a suggestion and we do not apply it everywhere we will get many suggestions and then we need to use #pragma warning disable IDE0045 // Convert to conditional expression to get rid of them, but that will be a lot.

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 27, 2019

@nopara73 @molnard I created this PR because conditional expression is used many times when possible in the entire repository, we can make that rule = false:error.
I will go with whatever you agree upon, for me what matters is consistency.

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

It is consistent: use conditional expression if the condition and the expressions are short as one line.
If it is longer than use if-else.

yahiheb and others added 3 commits Jul 30, 2019

@nopara73 nopara73 merged commit c7fd206 into zkSNACKs:master Jul 30, 2019

0 of 2 checks passed

CodeFactor Reviewing...
Details
Wasabi.Linux queued
Details

@yahiheb yahiheb deleted the yahiheb:conditional-expression-assignment-1 branch Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.