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

'null' should be on the right side of a binary expression. #5590

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Aug 2, 2020

Fixes #5589

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I agree 😄

@elachlan
Copy link
Contributor Author

elachlan commented Aug 2, 2020

One thing i'd like to point out is that readability has been reduced on some of the tests where the left nulls were aligned. So it isn't as easy to gather whats going on at a glance. But it does make more sense with this change.

@elachlan
Copy link
Contributor Author

elachlan commented Aug 7, 2020

@Forgind it would be great to get this merged before I start on the parentheses PR.

@Forgind
Copy link
Member

Forgind commented Aug 7, 2020

Team triage:
We talked about this PR with regard to the normal benefit of readability and the negative of making git blame harder and general churn plus the readability you noted with alignment in tests. If you find a rule in dotnet/runtime's coding styles (either by finding one that exists or adding a new one) that says this is beneficial, we would take it.

@elachlan
Copy link
Contributor Author

elachlan commented Aug 7, 2020

Team triage:
We talked about this PR with regard to the normal benefit of readability and the negative of making git blame harder and general churn plus the readability you noted with alignment in tests. If you find a rule in dotnet/runtime's coding styles (either by finding one that exists or adding a new one) that says this is beneficial, we would take it.

I have no skin in the game, so its really up to the team if they want it or not.

This might help:
dotnet/roslynator#183

@elachlan
Copy link
Contributor Author

elachlan commented Aug 8, 2020

I have created an issue in dotnet/runtime asking about the code-style change.
dotnet/runtime#40561

@benvillalobos
Copy link
Member

Seeing as #5646 hit many other PRs I'd like to close and reopen this just in case.

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

LGTM

@rainersigwald rainersigwald merged commit f2c4bfd into dotnet:master Aug 18, 2020
@rainersigwald
Copy link
Member

Thanks, @elachlan!

@elachlan elachlan deleted the 5589-null-on-right-side branch August 18, 2020 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'null' should be on the right side of a binary expression.
4 participants