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

[discovery] Add sort parameter for Discovery query #446

Merged

Conversation

maniax89
Copy link
Contributor

@maniax89 maniax89 commented Apr 27, 2017

Description

Adds the sort parameter to the Discovery query method -> see https://watson-api-explorer.mybluemix.net/apis/discovery-v1#!/Queries/get_v1_environments_environment_id_collections_collection_id_query for details

Checklist
  • npm test passes (tip: npm run autofix can correct most style issues)
  • tests are included
  • documentation is changed or added
  • link to public docs when adding new a service or new features for an existing service
New version_date Checklist
  • A new constant is avaliable with the version_date
  • The new constant has a comment that summarizes the changes and/or links to relevant doc pages
  • Any older version_date constants remain intact
  • The error message thrown if the service is created without a version_date indicates the new version_date constant
  • The example in the README includes the new version_date constant
  • Any relevant code in the examples/ folder has been updated to use the new version_date constant
  • Most tests are updated to the new version_date
  • 1-2 new tests are added that use the old version_date (optional, but preferred) - (no behavior is changed before/after the version date change to show any sort of different results)

@CLAassistant
Copy link

CLAassistant commented Apr 27, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@germanattanasio germanattanasio left a comment

Choose a reason for hiding this comment

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

LGTM

* @param {Number} [params.offset=0] For pagination purposes. Returns additional pages of results. Deep pagination is highly unperformant, and should be avoided.
* @param {String} [params.return] A comma separated list of the portion of the document hierarchy to return.
* @param {String} [params.sort] A comma separated list of fields in the document to sort on. You can optionally specify a sort direction by prefixing the field with - for descending or + for ascending. Ascending is the default sort direction if no prefix is specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is sorting supported in the newest version and should the default version string change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so its available in the newest version, just not sure what to do about the version string. i.e. it will work with any version date string since it isn't changing an existing parameter behavior, but adding a new one

@kognate
Copy link
Contributor

kognate commented Apr 27, 2017 via email

@maniax89
Copy link
Contributor Author

correct

@maniax89
Copy link
Contributor Author

At some point, version=2017-04-27 will do something on the query side when we do an api-breaking change. for now, it doesn't break existing behavior therefore the version date doesn't have any affect on the API call for this sort= parameter. But I can bump the default version to be consistent for how we would like it to work

@maniax89 maniax89 force-pushed the add_discovery_query_sort_param branch 3 times, most recently from 61aed8c to a5dc23f Compare April 27, 2017 19:52
@maniax89 maniax89 force-pushed the add_discovery_query_sort_param branch from a5dc23f to 99c0daa Compare April 27, 2017 20:10
@kognate kognate merged commit 38a2202 into watson-developer-cloud:master Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants