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

[3.0 Beta] Auto Generated API #830

Closed
wants to merge 1 commit into from
Closed

[3.0 Beta] Auto Generated API #830

wants to merge 1 commit into from

Conversation

nhtruong
Copy link
Collaborator

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Theo Truong <[email protected]>
@nhtruong nhtruong changed the title [3.0 Beta] Auto Generated API. 3.0 [3.0 Beta] Auto Generated API Aug 11, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks really good!

It is a little difficult to review the things that are generated vs. not. Do you think it's doable to split up at least the commits so that old API implementations and the generated code aren't in the way?

### Deprecated
### Removed
- *BREAKING* Removed support for API param aliases. That is, the API functions now only accept params with the exact names specified in the OpenSearch API specification.
- *BREAKING* Removed support for snake_cased API function names. All API functions are now camelCased only to conform to JavaScript naming conventions.
Copy link
Member

Choose a reason for hiding this comment

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

Create an UPGRADING.md that shows the changes with examples, e.g. https://github.com/opensearch-project/opensearch-py/blob/main/UPGRADING.md

*/

/*
* This file was generated from OpenSearch API Spec. Do not edit it
Copy link
Member

Choose a reason for hiding this comment

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

Should "Spec" be lowercased? ;)

/*
* This file was generated from OpenSearch API Spec. Do not edit it
* manually. If you want to make changes, either update the spec or
* the API generator.
Copy link
Member

Choose a reason for hiding this comment

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

Add something like "by running npm ..." or a mention of a doc like "see dev guide"

const updateByQueryRethrottleApi = require('./api/update_by_query_rethrottle');
const { kConfigErr, apiFunc } = require('./utils')
const kApiModules = Symbol('api modules')
const kApiLoader = Symbol('api loader')
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why these aren't named, e.g. this._api_loader?

sql: { get() { return this[kApiLoader]('./sql/_api') } },
tasks: { get() { return this[kApiLoader]('./tasks/_api') } },
transforms: { get() { return this[kApiLoader]('./transforms/_api') } },
})
Copy link
Member

Choose a reason for hiding this comment

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

It probably makes sense to have an explicit loadAll() as well to avoid warmup time during service. Doesn't need to be in this PR.

@nhtruong nhtruong closed this Aug 12, 2024
@nhtruong nhtruong deleted the api_3.0 branch August 12, 2024 20:00
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