-
Notifications
You must be signed in to change notification settings - Fork 2
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
Object-based scripting #106
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes them more discoverable in the IDE, and also simplifies discovery in case we want to generate some static docs automatically. Plus, this greatly simplifies implementation and type safety in the common case.
Simplifies matching
Includes a new test to verify percentage
dhleong
force-pushed
the
dhleong/object-based-scripting
branch
from
January 10, 2020 15:33
a9a9094
to
60e6796
Compare
Should make it very easy to generate docs using `extractFunctions`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a large refactor to how scripting functions are declared and discovered, using a reflection + annotation on actual object methods approach rather than manually registering untyped lambdas.
While we still lean on the Any type quite a bit here, having some type-safety of argument types significantly simplifies overload handling—while it is not 100% perfect, in most cases we can simply use normal method overloads to handle different argument types.
prompt()
, for example, goes from unpacking an array in awhen()
based on args and nesting anif
to disambiguate on type to two typed overloads (one calling through to the other, even) with a single "if" check testing for the presence of the optionalgroup
parameter (a normal, idiomatic thing to do!).We don't, unfortunately, support Kotlin's default parameter values just yet. It doesn't come up terribly often, but it might be nice, for example, with the prompt string to
input()
or theremap
boolean ofnormal()
. Such cases can generally be handled by overloads, however, and since we already includekotlin-reflect
(for some reason) it is something we could possibly support in the future.This change does sacrifice some performance—by invoking the methods with reflection we are always re-packaging the arguments array into another array. This article suggests we could improve performance using
LambdaMetafactory
but most of the scripting functions are unlikely to be in hot paths. Things that could be are the Buffer methods, but those are unaffected by this change, so trying to speed this up is probably a premature optimization.Future work might be able to take advantage of the changes here to generate offline scripting documentation.