Skip to content

Conversation

@exoego
Copy link
Contributor

@exoego exoego commented Jan 21, 2020

Copy link
Owner

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

Hey @exoego, thanks for the PR. Apart from the review comments, could you also add a test alongside and similar to 👇


Thanks!

}

/**
* Specifies to include documents without a value for a given source in the
Copy link
Owner

Choose a reason for hiding this comment

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

Change to Specifies whether to include documents without a value for a given source in the response. Defaults to false (not included).. Could you also wrap the comment at 80 characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 96fc517

src/index.d.ts Outdated
* in a document.
*
* Note: The `missing` option of the composite aggregation is deprecated in
* [Elastticsearch v6.0](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/breaking-changes-6.0.html#_literal_missing_literal_is_deprecated_in_the_literal_composite_literal_aggregation),
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* [Elastticsearch v6.0](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/breaking-changes-6.0.html#_literal_missing_literal_is_deprecated_in_the_literal_composite_literal_aggregation),
* [Elasticsearch v6](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/breaking-changes-6.0.html#_literal_missing_literal_is_deprecated_in_the_literal_composite_literal_aggregation),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 96fc517

src/index.d.ts Outdated
Comment on lines 4658 to 4659
* Note: The `missing` option of the composite aggregation is deprecated in
* [Elastticsearch v6.0](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/breaking-changes-6.0.html#_literal_missing_literal_is_deprecated_in_the_literal_composite_literal_aggregation),
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add this note here as well 👇

/**
* Missing specifies the value to use when the source finds a missing value
* in a document.
*
* @param {string} value
* @returns {ValuesSourceBase} returns `this` so that calls can be chained
*/
missing(value) {
this._opts.missing = value;
return this;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 96fc517

@sudo-suhas sudo-suhas merged commit 9d1e6b2 into sudo-suhas:master Jan 24, 2020
@exoego exoego deleted the composite-missing-bucket branch January 24, 2020 03:54
@sudo-suhas
Copy link
Owner

Thank you @exoego! Changes have been released in elastic-builder@2.5.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants