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

When using esb.sort().script(...) #78

Merged
merged 13 commits into from
Nov 20, 2018
Merged

Conversation

yoonhoGo
Copy link
Contributor

If esb.sort() is used without field parameter to use esb.sort().script(...), We should omit field parameter.
This case is expression in the script section of the document page.

If esb.sort() is used without `field` parameter to use esb.sort().script(...), We can omit `field` parameter.
This case is expression in the script section of [the document page](https://elastic-builder.js.org/docs/#sort).
@yoonhoGo
Copy link
Contributor Author

Reviewer: @sudo-suhas

@sudo-suhas
Copy link
Owner

Let's do the change in the following places to annotate the field argument to esb.sort() as optional:

  • * @param {string} field The field to sort on
  • elastic-builder/src/index.d.ts

    Lines 8261 to 8266 in 59c951d

    * @param {string} field The field to sort on
    * @param {string=} order The `order` option can have the following values.
    * `asc`, `desc` to sort in ascending, descending order respectively.
    */
    export class Sort {
    constructor(field: string, order?: string);
  • elastic-builder/src/index.d.ts

    Lines 8420 to 8424 in 59c951d

    * @param {string} field The field to sort on
    * @param {string=} order The `order` option can have the following values.
    * `asc`, `desc` to sort in ascending, descending order respectively.
    */
    export function sort(field: string, order?: string): Sort;

Additionally, in the sort constructor, _field property should only be set if it isn't nil.

src/index.d.ts Outdated
* Allows creating and configuring sort on specified field
* but It is required .script()
*/
export function sort(): Sort;
Copy link
Owner

Choose a reason for hiding this comment

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

Just annotating the field parameter for existing function and constructor declarations should be enough, no need to add the overriding function declaration.

src/core/sort.js Outdated
@@ -31,7 +31,8 @@ const invalidUnitParam = invalidParam(ES_REF_URL, 'unit', UNIT_SET);
* .query(esb.termQuery('user', 'kimchy'))
* .sort(esb.sort('post_date', 'asc'))
*
* @param {string} field The field to sort on
* @param {string=} field The field to sort on.
* If `.script()` function is used, It will be omitted.
Copy link
Owner

Choose a reason for hiding this comment

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

This could be reworded as

Suggested change
* If `.script()` function is used, It will be omitted.
* If a script is used to specify the sort order, `field` should be omitted.

Could you please do these changes in the 3 places? I apologise for the redundancy, I am planning to do something about it sometime soon.

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 so easy. 😄

Copy link
Owner

Choose a reason for hiding this comment

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

I agree that it's easy but it's also easy to miss and not obvious that the same changes have to made in multiple places. Nevertheless, I appreciate your patience and cooperation.

@sudo-suhas sudo-suhas merged commit c52ee2c into sudo-suhas:master Nov 20, 2018
@sudo-suhas
Copy link
Owner

Thank you @yoonhoGo for the PR! Changes merged in and released in v2.2.2 🎉

@yoonhoGo
Copy link
Contributor Author

Thank you @sudo-suhas. You're so very kind!

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