Skip to content

Some style rule updates with fixes to existing violations #4395

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

SuuperW
Copy link
Contributor

@SuuperW SuuperW commented Jul 9, 2025

Interestingly, IDE0038 Use pattern matching to avoid is check followed by a cast (without variable) doesn't work with dotnet build but does work inside Visual Studio.

There are quite a few other rules I find stupid (like preferring var over 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:

@SuuperW
Copy link
Contributor Author

SuuperW commented Jul 9, 2025

Okay so apparently the build checks for PRs will flag private unused members that are used via attributes+reflection, but dotnet build will not. So this needs more work.

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 dotnet build? EDIT: Oh it was probably because I made a local editorconfig that set its severity to none as a workaround for a VS bug. ...I thought I did a working build without that though. My bad.

@YoshiRulz
Copy link
Member

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 SuuperW force-pushed the misc_fixes branch 2 times, most recently from dae548b to 748bc99 Compare July 10, 2025 00:36
@SuuperW
Copy link
Contributor Author

SuuperW commented Jul 10, 2025

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).

@Morilli
Copy link
Collaborator

Morilli commented Jul 27, 2025

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 .git-blame-ignore-revs after.

Rest is fine.

@SuuperW
Copy link
Contributor Author

SuuperW commented Jul 29, 2025

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.

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?

@YoshiRulz

This comment was marked as resolved.

@SuuperW

This comment was marked as resolved.

@SuuperW SuuperW force-pushed the misc_fixes branch 2 times, most recently from fe7aaa9 to bd4cd1a Compare July 31, 2025 05:54
@YoshiRulz
Copy link
Member

I've pushed a bunch of code style commits to master, including 71d3d65 which addresses SA1508 (SA1509 seemed unhelpful), and ef9d64e which adds .editorconfigs for individual cores. Rebasing should make your diff much smaller now.

@SuuperW
Copy link
Contributor Author

SuuperW commented Aug 5, 2025

I've pushed a bunch of code style commits to master, including 71d3d65 which addresses SA1508 (SA1509 seemed unhelpful), and ef9d64e which adds .editorconfigs for individual cores. Rebasing should make your diff much smaller now.

There are a couple hundred fewer line removals now, but the vast majority of line changes are indentation changes which hasn't changed much.

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.

3 participants