Skip to content

Clean up stale code identified by static analysis#135

Open
robinplace wants to merge 7 commits intothomaswp:integrationfrom
robinplace:clean-up-stale
Open

Clean up stale code identified by static analysis#135
robinplace wants to merge 7 commits intothomaswp:integrationfrom
robinplace:clean-up-stale

Conversation

@robinplace
Copy link
Copy Markdown
Collaborator

@robinplace robinplace commented Mar 15, 2026

Static analysis with Roslynator reveals quite a few unused files, classes, & methods. Removing these means less total code to keep track of for everyone going forward.

Copy link
Copy Markdown
Collaborator Author

@robinplace robinplace Mar 15, 2026

Choose a reason for hiding this comment

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

Serializer.cs is FileIO.cs but with unused classes removed. I renamed it because all it does now is serialize-deserialize.

@robinplace robinplace requested a review from thomaswp March 17, 2026 23:40
@robinplace robinplace force-pushed the clean-up-stale branch 3 times, most recently from 469c348 to 50d6786 Compare March 17, 2026 23:54
@thomaswp thomaswp changed the base branch from master to integration March 21, 2026 22:54
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Makes me a bit sad to see it go, since this was the original way the mod worked, but there's no active plans to add an actual replay system into the mod, and if we did we'd want to reimplement it anyway.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is quite useful code. It doesn't go in the deployed mod, but it's useful for dev. Is there a nice way to mark that for your static analysis tool?

}
}

public void FinishFullTickIfNeededAndThen(Action action)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I realize this is actually the method that probably should used, rather than calling the TickingService's directly. Otherwise saving during a pause wouldn't happen until the next tick. So we should keep that

Copy link
Copy Markdown
Owner

@thomaswp thomaswp left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. I've regretted not starting with a good import organization setting on. I suppose we should be consistent - I'll follow up with a comment about that.

Two main changes (see comments):

  • ReflectionUtils is still used, just only in dev
  • FinishFullTickifNeededAndThen should be used.

I can fix both of those and merge.

@thomaswp
Copy link
Copy Markdown
Owner

So I assume for imports the simplest thing is just to be consistent in using VS's built-in remove unused and sort actions on save? That's harder to enforce in the repo. I think it's probably good enough to just put that in the readme to keep overhead lower and avoid external tools, but let me know if you have a strong opinion otherwise.

@@ -697,9 +692,9 @@ public class GameSaverSavePatcher
static bool Prefix(GameSaver __instance, QueuedSave queuedSave)
{
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Here I've changed it to use the ReaplyService method directly so it's no longer unused.

@thomaswp
Copy link
Copy Markdown
Owner

Hmm, so unfortunately your static analysis tool seems to have been far too overaggressive on removing imports, especially those needed for HarmonyPatches, both the Harmony import itself and also the classes used in it. So right now this PR isn't usable. Did you actually get it to compile on your end?

I see a few options:

  • Manually go back and add all the deleted imports that were missing.
  • Start a new PR using a more conservative cleanup tool, keeping the modifications I made here for "dead code" that's still useful.

@thomaswp
Copy link
Copy Markdown
Owner

This is also a note to self that once this is cleaned up, I should test to make sure that saving on pause doesn't cause any unforeseen issues with the change I made...

@robinplace
Copy link
Copy Markdown
Collaborator Author

robinplace commented Mar 22, 2026

The way to enforce this going forward is definitely a CI job that runs against PRs. We can configure such that build & lint tests get run automatically on every incoming PR. That way authors get feedback & can fix issues themselves.

GitHub actions are free for public repositories & there's a ton we can do with them to take effort off your back (it will also catch if e.g. VS Code's built in import cleanup tool kills the build lol).

@thomaswp thomaswp mentioned this pull request Mar 22, 2026
4 tasks
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.

2 participants