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

Change QueryTemplateParser to use script naming convention #9995

Closed
colings86 opened this issue Mar 5, 2015 · 5 comments
Closed

Change QueryTemplateParser to use script naming convention #9995

colings86 opened this issue Mar 5, 2015 · 5 comments
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache discuss >enhancement

Comments

@colings86
Copy link
Contributor

At the moment the QueryTemplateParser uses query, file and id for inline, file and indexed scripts respectively. Since the naming convention for scripts is now script, script_file and script_id (enforced by using the ScriptParameterParser) we should change this API to follow the same convention. We may want to keep query as an option for inline scripts as it makes sense in this context but we should support script too. file and id should be replaced with script_file and script_id.

Original comment in: https://github.com/elasticsearch/elasticsearch/pull/9992/files#r25855691

@clintongormley
Copy link

file and id should be replaced with script_file and script_id.

Or query_file and query_id? Not sure it is obvious to users that "templates" are a form of scripting.

Also, we can leave file and id and just mark them as deprecated, no?

@colings86
Copy link
Contributor Author

@clintongormley yep, good point. It should be noted that ScriptParameterParser can still be used for this since it accepts arbitrary prefixes as well as the default script prefix. As for deprecation, yes we can but as will the script options we could also deprecate in 1.5 and remove in 2.0?

@javanna
Copy link
Member

javanna commented Mar 20, 2015

I think we should come up with a Script construct that holds all the relevant parsing code, which needs to be streamlined whenever we support scripts in the codebase. Script would then hold 'lang', 'type', the script itself etc. and could be passed as a single argument to ScriptService etc. instead of the different arguments that we currently have.

@uboness
Copy link
Contributor

uboness commented Mar 21, 2015

+1 on the Script construct... it should also implement ToXContent.

@clintongormley
Copy link

Closed in favour of #11164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache discuss >enhancement
Projects
None yet
Development

No branches or pull requests

4 participants