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

Add augmentation #19003

Merged
merged 4 commits into from
Jun 21, 2016
Merged

Add augmentation #19003

merged 4 commits into from
Jun 21, 2016

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Jun 21, 2016

This adds the ability to have additional methods to whitelisted classes. These are just static methods taking the receiver as the first parameter. There isn't a performance hit or anything.

I added a few of groovy's enhancements to the collections api (e.g. maps n lists) to make those easier to work with. These are things such as each, collect, and so on that can be less verbose than java streams apis. Closure parameters were just swapped out for the appropriate java functional interface.

We may want to do more of them, candidates IMO would be around CharSequence/String/Pattern, to make manipulation easier for update scripts/ingest/etc.

This replaces the previous aliases functionality (which didn't work quite right in some cases) and ensures that these methods work in all situations (e.g. target of a method reference and so on). The aliases are just accomplished this way instead.

@nik9000
Copy link
Member

nik9000 commented Jun 21, 2016

CharSequence/String/Pattern

Indeed! I can have a look at some of those once this is in. Lots of them aren't really good, but a few will be helpful.

@@ -217,7 +219,15 @@ public MethodType getMethodType() {
// otherwise compute it
final Class<?> params[];
final Class<?> returnValue;
if (Modifier.isStatic(modifiers)) {
if (augmentation) {
// virtual/interface method disguised as static
Copy link
Member

Choose a reason for hiding this comment

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

static method disguised as virtual?

@rmuir
Copy link
Contributor Author

rmuir commented Jun 21, 2016

also for the map apis, we should make a choice whether they should be BiFunction/BiPredicate like i have them here, or just Function/Predicate but taking the Map.Entry.

Its the difference between:

map.collect((key,value) -> key + value)

and

map.collect(entry -> entry.key + entry.value)

I really have no strong opinions, but in the scripting api its nearly impossible for me to tell what is a map and what is a list. By requiring lambdas on maps to take two parameters (key, value), if things are wrong it will be very clear. This is also consistent with the apis java has on Map. On the other hand it means parentheses...

@jdconrad
Copy link
Contributor

LGTM! @rmuir Thanks for doing this!

@jdconrad jdconrad mentioned this pull request Jun 21, 2016
18 tasks
@nik9000
Copy link
Member

nik9000 commented Jun 21, 2016

I prefer the BiFunction version because it has you name the key and value something interesting and you rarely ever need a Map.Entry from one of those.

@rmuir
Copy link
Contributor Author

rmuir commented Jun 21, 2016

ok lets stick with it. we can always change it. The BiFunction has the advantage of providing the clearest error messages and least confusion, because arity gives us that (even if types are murky).

@rmuir rmuir merged commit f70211d into elastic:master Jun 21, 2016
@clintongormley clintongormley changed the title painless: add augmentation Add augmentation Jun 22, 2016
@clintongormley clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v5.0.0-alpha4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants