Improve docs and usability of EvaluatorBuilder #197
Conversation
odenix
commented
Feb 18, 2024
odenix
commented
- extend, update, and reword docs (inspired by pkl-lang.org and Project.pkl docs)
- avoid duplication of docs by referring from add/get methods to set methods
- add a few add/get/set methods to improve consistency and documentability
- default to empty instead of null StackFrameTransformer to improve usability
- do not change builder state in build()
- require non-empty allowedModules
- require null securityManager in setRootDir()
- improve some error messages
- extend, update, and reword docs (inspired by pkl-lang.org and Project.pkl docs) - avoid duplication of docs by referring from add/get methods to set methods - add a few add/get/set methods to improve consistency and documentability - default to empty instead of null StackFrameTransformer to improve usability - do not change builder state in build() - require non-empty allowedModules - require null securityManager in setRootDir() - improve some error messages
There was a problem hiding this comment.
Some nits, but otherwise, this looks good!
Passing over to @stackoverflow for review
| /** | ||
| * Sets the URI patterns that determine if a module import is allowed. | ||
| * | ||
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri}, |
There was a problem hiding this comment.
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri}, | |
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri"}, |
| /** | ||
| * Sets the module trust levels that determine if a module import is allowed. | ||
| * | ||
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri}, |
There was a problem hiding this comment.
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri}, | |
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri"}, |
| * Sets the URI patterns that determine if a module import is allowed. | ||
| * | ||
| * <p>Pkl code can import a module with {@code import "moduleUri"}, {@code amends "moduleUri}, | ||
| * {@code extends "moduleUri"}, or an {@code import("moduleUri")} expression. The import is |
There was a problem hiding this comment.
There's also import*(). How about:
| * {@code extends "moduleUri"}, or an {@code import("moduleUri")} expression. The import is | |
| * {@code extends "moduleUri"}, or an {@code import("moduleUri")} or {@code import*("moduleUri")} expression. The import is |
Or, to be more general:
"Pkl code can import a module by using an amends or extends header, or with an import expression."
(same comment applies elsewhere)
| * <p>Pkl code can read a resource with {@code read(resourceUri)}. The read is allowed only if | ||
| * {@code resourceUri} matches at least one of the given patterns. |
There was a problem hiding this comment.
[nit] for consistency sake with modules, this should be quoted.
| * <p>Pkl code can read a resource with {@code read(resourceUri)}. The read is allowed only if | |
| * {@code resourceUri} matches at least one of the given patterns. | |
| * <p>Pkl code can read a resource with {@code read("resourceUri")}. The read is allowed only if | |
| * {@code resourceUri} matches at least one of the given patterns. |
There's also read?() and read*(). So, this might be better worded as:
"Pkl code can read a resource with a read expression"
There was a problem hiding this comment.
If you go with "Pkl code can read a resource with a read expression", or something of the sort, it's a good idea to link to the language reference for reads: https://pkl-lang.org/main/current/language-reference/index.html#resources
| if (allowedResources.isEmpty() && allowedModules.isEmpty()) { | ||
| throw new IllegalStateException("No security manager set."); | ||
| if (allowedModules.isEmpty()) { | ||
| throw new IllegalStateException("allowedModules cannot be empty"); |
There was a problem hiding this comment.
It would also error if pkl: is not part of the allowed modules.
Not actually sure that this check is that meaningful. It's not good enough as it stands, and the error from the evaluator is already pretty clear.
| * <p>Pkl code can read a resource with {@code read(resourceUri)}. The read is allowed only if | ||
| * {@code resourceUri} matches at least one of the given patterns. |
There was a problem hiding this comment.
If you go with "Pkl code can read a resource with a read expression", or something of the sort, it's a good idea to link to the language reference for reads: https://pkl-lang.org/main/current/language-reference/index.html#resources
| public List<ModuleKeyFactory> getModuleKeyFactories() { | ||
| return moduleKeyFactories; | ||
| } | ||
|
|
||
| /** | ||
| * Adds a {@code ResourceReader} object that handles resource reads. |
There was a problem hiding this comment.
nit: This phrase is tautological. I'd just shorten it to Adds a {@code ResourceReader}. Same for addResourceReaders.
There was a problem hiding this comment.
My aim throughout was to make the overview comment (i.e., first sentence) useful. This worked out better in some cases than in others, but I tried to stay consistent. I felt that commenting the method addFoo(Foo foo) with Adds a {@code Foo} is useless because this is obvious from the method signature.
