-
Notifications
You must be signed in to change notification settings - Fork 59
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
AF-72 - App Framework require expressions should support debugging an… #59
Conversation
@@ -93,29 +97,17 @@ public AppFrameworkServiceImpl(AllAppTemplates allAppTemplates, AllAppDescriptor | |||
this.javascriptEngine = new ScriptEngineManager().getEngineByName("JavaScript"); | |||
this.allUserApps = allUserApps; | |||
|
|||
|
|||
// there is surely a cleaner way to define this utility function in the global scope |
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 is my version of a "cleaner way". This eliminates the global scope entirely, as it introduces unnecessary complexity for what I expect is negligible performance gain, and just keeps all of the various functions as their source strings, which are appended to the script that is executed each time.
This also moves the definition of these functions out of this very-hard-to-understand-and-maintain string concatenation and into javascript resource files that can benefit from syntax highlighting, code completion, formatting, and all of the other benefits that come with it.
} | ||
public void addRequireExpressionScript(String key, String value) { | ||
requireExpressionScripts.put(key, value); | ||
} |
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 method allows implementations to register new helper functions in their activators.
} | ||
} | ||
script.append(System.lineSeparator()); | ||
return script.toString(); |
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 logic in this method is more-or-less unchanged - the difference is that instead of adding things to the javascript engine, this is creating a string which the javascript engine can evaluate, but also which could be inspected to provide information about the execution context.
String script = getRequireExpressionContext(contextModel) + System.lineSeparator() + requireExpression; | ||
return javascriptEngine.eval(script).equals(Boolean.TRUE); | ||
} | ||
} |
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 simply adds a new method that mirrors the existing one, but which does not swallow exceptions. The existing method delegates to this and handles the exception to retain backwards compatibility.
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, thanks @mseaton !
…d customization
See: https://openmrs.atlassian.net/browse/AF-72