-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
High level REST client parameters look wrong due indirectness and side effects #39666
Comments
Pinging @elastic/es-core-features |
This makes way more sense. Ive added good first issue to this in case someone wants to pick it up. Feel free to assign me to a PR if anyone does pick it up. |
@hub-cap I am on it. Will be sending a PR soon. |
@vedantseta have you started working on this yet? if not i would like to pick this up. |
@jasontedor please check the above merge request. |
@kutty-kumar are you still working on this? Are you good with me picking this one up? |
@jasontedor @hub-cap Hello is anyone working on this issue? if not I would like to work on this. Also could you please guide me a little bit how should I get started? 😃 |
@ojasgulati There is a PR in progress; let us give the author of that PR a chance to bring it to completion. |
This commit will use parameters in request object
Using the high-level request client, this is how code to build a request with parameters appears today:
This looks wrong because
parameters
appears to be unused. That is, we create aRequestConverter.Params
object, mutate it, and then it appears nothing happens to it. This code is not broken though, because what is happening behind the scenes is thatrequest
is being mutated when methods onparameters
are invoked. This indirectness and methods having side effects on another object lead to code that does not appear correct at first glance, and is more confusing than it should be; it looks wrong. Instead, I propose an alternative like:This makes apparent that the
parameters
are actually used, and that they have an effect on therequest
object through explicit invocation. I believe this makes it more transparent what is occurring.The text was updated successfully, but these errors were encountered: