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.

Sign up for GitHub

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

Jump to bottom

Implement public layer for Aggregations API #234

Merged
merged 3 commits into from
May 31, 2023
Merged

Implement public layer for Aggregations API #234

merged 3 commits into from
May 31, 2023

Conversation

markaya
Copy link
Contributor

@markaya markaya commented May 30, 2023

Summary:

  • Implemented public classes to be used by users when working with Aggregations

@markaya markaya force-pushed the task-make-public-aggregations-api branch from 2fc5c83 to f854b3b Compare May 30, 2023 12:20
dbulaja98
dbulaja98 requested changes May 30, 2023
View reviewed changes
Copy link
Collaborator

@dbulaja98 dbulaja98 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Consider different naming, we currently have AggregationResult and AggregateResult.

modules/library/src/main/scala/zio/elasticsearch/aggregation/AggregationResult.scala Outdated
* limitations under the License.
*/

package zio.elasticsearch.aggregation
Copy link
Collaborator

@dbulaja98 dbulaja98 May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe put this file to results package.

@markaya markaya force-pushed the task-make-public-aggregations-api branch from f854b3b to 785056a Compare May 31, 2023 10:34
drmarjanovic
drmarjanovic requested changes May 31, 2023
View reviewed changes
modules/library/src/main/scala/zio/elasticsearch/result/AggregationResult.scala Outdated
) extends AggregationResult {

def subAggregationAs[A <: AggregationResult](aggName: String): Option[A] =
subAggregations.flatMap(_.get(aggName).map(a => a.asInstanceOf[A]))
Copy link
Member

Choose a reason for hiding this comment

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

Can you use _.asInstanceOf[A]? What if this throws an error? Should we use match and then return None if wrong instance?

@drmarjanovic drmarjanovic merged commit 889859d into main May 31, 2023
@drmarjanovic drmarjanovic deleted the task-make-public-aggregations-api branch May 31, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbulaja98 dbulaja98 dbulaja98 requested changes

@drmarjanovic drmarjanovic drmarjanovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markaya @drmarjanovic @dbulaja98