-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
More robust multiexpression filtering #206
Conversation
https://github.com/onthegomap/planetiler/actions/runs/2273002294 ℹ️ Base Logs 891537e
ℹ️ This Branch Logs 04adb58
|
if (!always.isEmpty()) { | ||
LOGGER.warn("{} expressions will be evaluated for every element:", always.size()); | ||
for (var expression : always) { | ||
LOGGER.warn(" {}", expression.expression.generateJavaCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix should be to surround the warn call with if(LOGGER.isWarnEnabled()) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few cases like this that I think I want to tackle in one pass (and leave highlighted so I don't miss one). I'd prefer to go the route of a logger framework that takes a lambda that only gets invoked if that level is needed. There's also some other logging issues to clear up like core
should just depend on slf4j and the runnable thing (dist
module or a project that depends on this) should bind slf4j to whatever implementation it wants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good plan. Looks like https://github.com/kwon37xi/slf4j-lambda exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also slf4j v2 api will support lambdas. I'd like to use that but it's been in alpha stage since 2019...? https://mvnrepository.com/artifact/org.slf4j/slf4j-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a logging framework, how bad could alpha be? 🤔
Fixes #202 |
planetiler-core/src/main/java/com/onthegomap/planetiler/expression/MultiExpression.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/expression/Expression.java
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for improving this logic!
thanks for the review! glad to fix these issues now before we start to handle arbitrary expressions coming in from user-defined schema configs. |
Proposed more robust fix for logic issues @ZeLonewolf has noticed in #205 #204 #203 #202 #201 and #200