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

[DOCS] Reformat avg bucket agg reference #69751

Merged
merged 6 commits into from
Mar 2, 2021
Merged

[DOCS] Reformat avg bucket agg reference #69751

merged 6 commits into from
Mar 2, 2021

Conversation

jrodewig
Copy link
Contributor

@jrodewig jrodewig commented Mar 1, 2021

Reformats reference documentation for the average bucket aggregation.

I plan to use these docs as a template for other agg reference documentation.
Relates to #69215, #66208, #66205, #53304

Preview

https://elasticsearch_69751.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/search-aggregations-pipeline-avg-bucket-aggregation.html

@jrodewig jrodewig requested a review from a team March 2, 2021 13:57
@jrodewig jrodewig marked this pull request as ready for review March 2, 2021 13:58
@elasticmachine elasticmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Docs Meta label for docs team labels Mar 2, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

}
}
--------------------------------------------------
----
// NOTCONSOLE
Copy link
Member

Choose a reason for hiding this comment

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

I think its probably helpful to spend the extra lines to turn this into a CONSOLE example. We'll get the test from it and readers will know where the agg sits in the request tree.

Copy link
Member

Choose a reason for hiding this comment

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

Looking again, it really is a lot of lines. And we already say we'll have an example below. So I'm not really sure. I do like full examples, but this one would take up the whole screen......

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @nik9000. I considered either converting this to a full example OR simply moving the example below up. However, it can be difficult to spot where exactly the avg_bucket agg begins/ends. We can do callouts, but that requires a bit more parsing on the part of the reader. I imagine that'll be a problem for most of our pipeline aggs.

Copy link
Member

Choose a reason for hiding this comment

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

It is like you say, yikes. Do you think we could have the syntax block include a tagged region from the example below? That way at least it won't drift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea! I implemented this with 105e3b1 (and some cleanup with ee06c0d).

The tagged region comments are now visible in the example. I don't think there is a substitution combo that lets you get rid of them. However, I don't think that's necessarily a bad thing. It does a good job of calling out the agg configuration in context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding callouts for the tagged regions? Just something to explain to the users that the tags indicate the agg configuration but aren't actually part of the example.

Copy link
Contributor

@lockewritesdocs lockewritesdocs left a comment

Choose a reason for hiding this comment

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

Left a suggestion, but LGTM otherwise 👍

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I'm happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >docs General docs changes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Docs Meta label for docs team v7.11.2 v7.12.1 v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants