Skip to content

Conversation

@jimczi
Copy link
Contributor

@jimczi jimczi commented Aug 29, 2016

This change fixes an example in the docs that use += to add an element in a list even though this syntax is not accepted by painless.

@nik9000 nik9000 added >bug >docs General docs changes labels Aug 29, 2016
@nik9000
Copy link
Member

nik9000 commented Aug 29, 2016

This looks like an opportunity to get further on #18160! If you convert these to // CONSOLE instead of curl then gradle docs:check will actually make sure the snippets work. You might need // TESTSETUP or // TEST[continued] to make the generated tests make sense, but it is so worth it.

@jimczi
Copy link
Contributor Author

jimczi commented Aug 29, 2016

@nik9000 I am changing some of the internals for the update API in another PR. I'll add the console stuff in this PR if you don't mind:
#20166

@nik9000
Copy link
Member

nik9000 commented Aug 29, 2016

I don't mind!

@jimczi
Copy link
Contributor Author

jimczi commented Aug 29, 2016

@jdconrad out of curiosity we were wondering with @nik9000 if that is rejected because it would be slow ?

@jdconrad
Copy link
Contributor

jdconrad commented Aug 29, 2016

@jimferenczi It's not that it would be slower (for static types), but there are two downsides I see. The first one is added code complexity. For each overloaded operator there must be a section of specialized code to identify the list and call the appropriate method. We picked some of these that we saw as the most common/useful and decided to not have others. The second reason is added complexity for the user. Having more than one way of doing the same thing leads to the question of is one way better than the other. And at times one way is actually better than the other just because of the types the operator works on (specifically dynamic types) which can lead to even more confusion.

@jimczi jimczi merged commit accb636 into elastic:master Aug 30, 2016
@jimczi
Copy link
Contributor Author

jimczi commented Aug 30, 2016

Makes perfect sense, thanks @jdconrad

@jimczi jimczi deleted the painless_list_add branch February 3, 2017 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug >docs General docs changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants