-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Small fixes #8028
base: develop
Are you sure you want to change the base?
Small fixes #8028
Conversation
Hi! my 2 cents, I think 21 commits to change 15 files isn't great for commit history, wouldn't it be better to do a PR with a clean history to reflect what you want to change? For example, the same commit message is repeated many times and I don't think that adds value to the story. |
Hi! Ty for your advice. I squashed all these commits into one |
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 like small cleanup of is
operator usage, but please revert the StringBuilder
usage in ToString
methods.
Also don't worry about making too many commits in a PR, the maintainers can easily squash them down during the merge process 😉
No problem, I reverted StringBuilder usage |
@@ -6,6 +6,7 @@ | |||
using System.Diagnostics; | |||
using System.Runtime.CompilerServices; | |||
using System.Runtime.Serialization; | |||
using System.Text; |
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.
Missed reverting this one.
These changes are C# 5+ features. We really need to restore the C# 5 check from the ConsoleCheck csproj and re-run it to make sure that none slipped in while it was disabled. EDIT: reverted, and ran the checks. |
Like I feared, we already merged code that breaks this. I'll track that later to revert those changes. |
Some small fixes: