-
Notifications
You must be signed in to change notification settings - Fork 388
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
Standardize and Optimize Code with dotnet format #3754
base: master
Are you sure you want to change the base?
Conversation
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.
This would be a good idea to enable—after the codebase has been given a manual once-over. This way is like seeing a dead canary and deciding to pump clean air into the birdcage.
I don't think the order is that crucial. I did look briefly before and found I can definitely improve the codebase with a manual review in ways that the auto checks can't find with things like LINQ expressions, so I could work on that first though if preferred. This PR also doesn't strictly enforce following dotnet format as a pre-commit or CI check which could be desired if the manual combing is also in place. |
src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs
Outdated
Show resolved
Hide resolved
046ff90
to
3053b63
Compare
Would it be helpful to add some info about the formatting style to the CONTRIBUTING file, and also the suggested / recommended way of checking for the formatting style based on the outputs of the |
Definitely would be helpful to add that to Contributing however "format" is a bit misleading here, this PR is less intended to change formatting standards like indent style and more making use of new language features where the format command happens to know how to implement them automatically. |
I see. I look forward to seeing the new info added to CONTRIBUTING. |
3053b63
to
09422e1
Compare
@@ -52,7 +52,7 @@ public virtual bool StartsFromSavestate | |||
public bool StartsFromSaveRam | |||
{ | |||
// ReSharper disable SimplifyConditionalTernaryExpression | |||
get => Header.TryGetValue(HeaderKeys.StartsFromSaveram, out var s) ? bool.Parse(s) : false; |
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.
These should be reverted, as even though it's technically a "simplification" it makes the code less readable. The resharper disable is already evident of this.
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 disagree on this one, usually something is less readable/over complicated if you're returning true in the true condition of a ternary or likewise false in the false condition, but if someone is already intentionally ignoring the optimization then sure we can revert it
bf09f1e
to
46e988b
Compare
Rebased on Morilli's PR and reran against the analyzer setting changes. I think something might be wrong with the IDE0007/IDE0008 var vs explicit settings still, will play with that as I have time. |
I think the main issue here is that a lot of the inspections at Blanket-applying those everywhere unconditionally may not be desired in all cases. |
for sure I'll toy with it more, I'll probably rerun it at warning level and retune individual rule warning levels if I see that missing more than I think people would agree on |
46e988b
to
90d2473
Compare
90d2473
to
e6ecb63
Compare
Follow up on #3681
Proposes merging the corrected output of
dotnet format style -v detailed --severity info
, optimizing and/or simplifying code per dotnet standards and removing unused code.Implements or standardizes the following:
|=
,&=
,>>=
,<<=
,++
,--
,+=
,-=
,%=
,??=
operator usageis
andis not
keywords in favor of==
and!=
where relevant?.
potential outlying changes:
There's a change to RCheevos.cs that looks possibly intended to be the existing way
Some removals of unused code in the Amstrad CPC, Commodore 64, PC Engine, and Sinclair Spectrum CPU cores may not make sense (it looks like a bunch of cpu registers end up unused)
Check if completed: