Create public methods for script and inner hits #216
Conversation
markaya
force-pushed
the
task-create-public-methods-for-Script-and-InnerHits
branch
from
fdcb9ed
to
de9f324
Compare
markaya
force-pushed
the
task-create-public-methods-for-Script-and-InnerHits
branch
from
de9f324
to
7d38125
Compare
|
|
||
| def name(value: String): InnerHits = | ||
| InnerHits(name = Some(value)) | ||
| def withName(value: String): InnerHits = |
There was a problem hiding this comment.
Why is this changed? Not sorted.
There was a problem hiding this comment.
It does not make sense to me when creating new InnerHits to say just InnerHits.name(...), withName made more sense. I would consider doing same for form and size. What do you think?
There was a problem hiding this comment.
I think we are trying to avoid with prefix. @arnoldlacko?
There was a problem hiding this comment.
Maybe we could drop all these and just have InnerHits.empty.name(...).from(...)?
| @@ -57,6 +57,6 @@ private[elasticsearch] final case class Script( | |||
| } | |||
|
|
|||
| object Script { | |||
| def apply(source: String): Script = | |||
| def from(source: String): Script = | |||
There was a problem hiding this comment.
Why use from instead of apply?
There was a problem hiding this comment.
We agreed earlier that overriding apply method is not something we want to be doing as it makes false feeling that Script has only one field.
There was a problem hiding this comment.
I would consider source as a required param. And if we have Script("source") then we can consider it as a wrapper on the string. I would use apply.
There was a problem hiding this comment.
Sure. I will change it to apply.
drmarjanovic
deleted the
task-create-public-methods-for-Script-and-InnerHits
branch
Summary:
Implemented idea for creating
ElasticUtilitiesobject that would contain methods to create objects such asScriptorInnerHitsthat do not belong to any other objects.