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

Improve docs and usability of EvaluatorBuilder #197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

translatenix
Copy link
Contributor

  • 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
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <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},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <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
Copy link
Contributor

@bioball bioball Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also import*(). How about:

Suggested change
* {@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)

Comment on lines +297 to +298
* <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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] for consistency sake with modules, this should be quoted.

Suggested change
* <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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Leaving some comments. Nothing major.

Comment on lines +297 to +298
* <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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This phrase is tautological. I'd just shorten it to Adds a {@code ResourceReader}. Same for addResourceReaders.

Copy link
Contributor Author

@translatenix translatenix Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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