Skip to content

Conversation

@Jonathing
Copy link
Member

Good morning.

This PR changes a few things in Log Utils to revolve around its global usage even though it can now have instance loggers. Specifically, indentation and capturing are now global. While they can be controlled using instance methods defined in Logger, they affect the state of the logger throughout execution.

As an example, if we are not capturing, and a Logger.create("SrgUtils") is created, the log might look something like this:

Generating Sources
  rename
    [SrgUtils] Failed to read name

@Jonathing Jonathing requested a review from LexManos October 27, 2025 12:17
@Jonathing Jonathing self-assigned this Oct 27, 2025
@PaintNinja
Copy link
Contributor

Creating a separate logger to mutate the global one seems a bit weird, maybe a static push(String) method would make more sense?

@Jonathing
Copy link
Member Author

I can make the indentation and capture methods static. I only left them virtual so you can access them from the instance as a sort of convenience.

@PaintNinja
Copy link
Contributor

Java supports calling static methods from an instance

@Jonathing
Copy link
Member Author

Not for interfaces. Logger is an interface.

@LexManos
Copy link
Member

Having instance loggers be attached to the static loggers seem a bit weird. I would prefer having it be opt in ala:

// Do some SrgUtilsStuff
StaticLogger.push();
srglogs.forEach(StaticLogger::log)
StaticLogger.pop();

But honestly, this looks fine to me as long as it works.

@Jonathing
Copy link
Member Author

Everything works, yes. And I can add a StaticLogger in another update without needing to break the API in Logger. Would that be acceptable?

@Jonathing
Copy link
Member Author

I am merging this immediately. If there are design choices we want to rework, we can do so in a later update.

@Jonathing Jonathing force-pushed the feat/jonathing/log-utils/0.5 branch from b4a046d to 2e23158 Compare October 29, 2025 16:47
@Jonathing Jonathing merged commit 2e23158 into MinecraftForge:master Oct 29, 2025
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