-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[incubator-kie-drools-6016] Emit a warning when an eval is improperly… #6024
[incubator-kie-drools-6016] Emit a warning when an eval is improperly… #6024
Conversation
… and unnecessarily used
public static void logWarnIfImproperEval(EvalCondition evalCondition, String evalExpression) { | ||
if (warnLogCounter == 10) { | ||
warnLogCounter++; | ||
LOG.warn("More eval warnings will be suppressed..."); | ||
return; | ||
} else if (warnLogCounter > 10) { | ||
return; // avoid flooding the logs | ||
} |
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.
Log a maximum of 10 warnings, because I guess if a user likes to use eval, there would be tons of warnings. 10 logs would be enough to get attention.
for (Declaration declaration : evalCondition.getRequiredDeclarations()) { | ||
if (declaration.getPattern() != null) { | ||
sb.append("'"); | ||
sb.append(declaration.getIdentifier()); | ||
sb.append("' comes from previous pattern '"); | ||
String className = declaration.getPattern().getObjectType().getClassName(); | ||
sb.append(className.substring(className.lastIndexOf('.') + 1)); | ||
sb.append("'. "); | ||
} | ||
} |
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.
The criteria of the warning is "If the eval uses a declaration which comes from previous pattern". I guess, most of eval usages meet the case. Do you think of other criteria? @mariofusco
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.
No, this is also what I had in mind. I cannot think to any other condition at the moment.
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.
Thanks, I'll go with it.
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.
LGTM
… and unnecessarily used (apache#6024)
… and unnecessarily used (apache#6024)
… and unnecessarily used
Issue: