Some style rule updates with fixes to existing violations #4395
Conversation
|
Okay so apparently the build checks for PRs will flag private unused members that are used via attributes+reflection, but EDIT: Okay but what about BizHawk.Emulation.Cores/Consoles/Nintendo/GBHawk/Audio.cs(1235,16): 'Audio.GetSamples'? Why isn't that flagged when I run |
|
Oh, I have a local branch which cleans up mixed tabs+spaces, let me dust that off. You shouldn't try to remove unused fields in at least the Cores project. Assume the core maintainer is in the middle of a refactoring. |
SuuperW
force-pushed
the
misc_fixes
branch
2 times, most recently
from
dae548b to
748bc99
Compare
|
There were some violations (including some with explicit suppression) of unused/unread members that were actually used but only via reflection. I refactored some stuff to fix these, using less reflection. Old code is deleted, but maybe it shouldn't be as that breaks external tools (until they refactor themselves, which isn't hard). |
|
428d1fe is a breaking change and should not be in this PR imo as it's less a style change and more an actual functional change. e726472 looks fine and while technically breaking is very easy to fix. I just wonder: Will this break existing external tools silently or does it error on load? In any case this change should be communicated clearly in changelog and whatever. 95b70f2 looks fine (i've definitely looked at every single line change) but should be added to Rest is fine. |
Yeah I wasn't sure if either of these belonged here. They're both easy updates for external tools to make. If this PR is otherwise good to merge I'll remove one or both and do in-source suppression. Is there a place I can put a note about these changes for changelog? |
| @@ -17,9 +17,6 @@ namespace BizHawk.Client.Common | |||
| [Description("A library for manipulating the EmuHawk client UI")] | |||
| public sealed class ClientLuaLibrary : LuaLibraryBase | |||
| { | |||
| [RequiredService] | |||
| private IEmulator Emulator { get; set; } | |||
There was a problem hiding this comment.
Looks like the core is accessed via this.MainForm instead for some reason?
There was a problem hiding this comment.
Using the Emulator property is better imo.
I'm not sure if Emulator ever was used, but the current uses of MainForm.Emulator were new methods added after the Emulator property, so that's weird.
I'll make it use the Emulator property instead of deleting it.
src/BizHawk.Client.EmuHawk/tools/Debugger/AddBreakpointDialog.cs
Outdated
Show resolved
Hide resolved
| //{ | ||
| // var tens = Math.DivRem(n, 10, out var ones); | ||
| // return (byte)((tens << 4) | ones); | ||
| //} |
There was a problem hiding this comment.
Just remove this one since it's redundant with with BCD2.IntToBCD (DiscTypes.cs)
There was a problem hiding this comment.
Will also remove the unused public BCD_Byte below it, since it actually just does the same thing and has the same visibility as BCD2.
|
I checked upstream and Which rule was flagging the blank lines at the end of scopes? |
"Fix violations of SA1508 and SA1509 (braces should [not] be preceded by blank line). Don't enable them because they flag some silly things." |
…fix (or ignore) violations. Don't bother with a couple of particularly offensive cores. A few non-trivial violations remain.
…e reflection is evil.
…ast violations of IDE0051 and IDE0052.
… blank line) Don't enable them because they flag some silly things.
Interestingly, IDE0038
Use pattern matching to avoid is check followed by a cast (without variable)doesn't work withdotnet buildbut does work inside Visual Studio.There are quite a few other rules I find stupid (like preferring
varover explicit types; explicit types are superior) but other people probably disagree so I just did the ones I think pretty much anyone would agree with.While fixing mixed use of tabs and spaces, a few files had majority spaces but a few lines with tabs. They're all tabs now. There are still files that use spaces exclusively.
Check if completed: