Skip to content
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

Use proper logging library #29

Merged
merged 8 commits into from
Mar 10, 2024
Merged

Conversation

NebelNidas
Copy link
Contributor

@NebelNidas NebelNidas commented Mar 30, 2023

Replaces all System.out calls with usages of a proper logger library.
Initially I wanted to use SLF4J's API with Log4j as implementation, but I couldn't get it to work with the Java module system, so I just used Log4j's API as well. Now uses SLF4J as API and Tinylog as implementation.
Also fixes some inconsistent capitalization of logged sentences.

@NebelNidas NebelNidas changed the title Logging library Use proper logging library Mar 30, 2023
@NebelNidas NebelNidas marked this pull request as draft March 30, 2023 18:07
src/main/java/matcher/srcprocess/Cfr.java Outdated Show resolved Hide resolved
src/main/java/matcher/type/Analysis.java Show resolved Hide resolved
src/main/java/matcher/serdes/MatchesIo.java Outdated Show resolved Hide resolved
src/main/java/matcher/gui/Gui.java Outdated Show resolved Hide resolved
src/main/java/matcher/Util.java Outdated Show resolved Hide resolved
@liach
Copy link
Contributor

liach commented Apr 11, 2023

Instead of using apache with those unnecessary and bug-prone "features", how about we use System.Logger and System.getLogger instead, available since JDK 9?

@NebelNidas
Copy link
Contributor Author

NebelNidas commented Apr 12, 2023

On second thought (and after seeing its JAR size), I agree that Log4j is indeed overkill for Matcher. I'm not a huge fan of System.Logger though, and since I want to split Matcher into a core (Java 8 based) and a gui (Java 17 based) module later down the road, using Java 9 features wouldn't work out anyway. I think I'm going to go with SLF4J and Tinylog for now

@liach
Copy link
Contributor

liach commented Apr 12, 2023

On second thought (and after seeing its JAR size), I agree that Log4j is indeed overkill for Matcher. I'm not a huge fan of System.Logger though, and since I want to split Matcher into a core (Java 8 based) and a gui (Java 17 based) module later down the road, using Java 9 features wouldn't work out anyway. I think I'm going to go with SLF4J and Tinylog for now

Downgrading Matcher from Java 17 to 8, which already has premier support ended, isn't quite a good idea.

Matcher/build.gradle

Lines 36 to 45 in f01ed5a

java {
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
if (!JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_17)) {
toolchain {
languageVersion = JavaLanguageVersion.of(17)
}
}
}

Your code using matcher directly should migrate to Java 17. 8 is sweet for those sun package stuff and unrestricted reflection to change final fields, but you shouldn't have relied on them in the first place.

@NebelNidas
Copy link
Contributor Author

NebelNidas commented Apr 12, 2023

I completely forgot that Matcher uses the Java module system already, so yeah, I should target Java 11 instead. I don't want to force library consumers into requiring Java 17 just yet, especially since Matcher's core functionality doesn't use any Java 12+ features anyway. Player can always bump it later if required.

@NebelNidas NebelNidas force-pushed the logging-library branch 2 times, most recently from e9ca8fa to 4fb0d13 Compare April 20, 2023 06:20
This was referenced Apr 20, 2023
@NebelNidas NebelNidas mentioned this pull request Jun 25, 2023
@NebelNidas NebelNidas force-pushed the logging-library branch 2 times, most recently from 88ff6af to 07a990e Compare October 10, 2023 14:08
@NebelNidas NebelNidas marked this pull request as ready for review October 11, 2023 07:26
@sfPlayer1 sfPlayer1 merged commit da3d2b9 into sfPlayer1:master Mar 10, 2024
1 check passed
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