Skip to content

Factor mustache -> modules/lang-mustache #15328

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

Merged
merged 12 commits into from
Dec 10, 2015
Merged

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Dec 9, 2015

This is the last of the scripting engines to refactor to a module.

It removes a third party dependency from core, removes third party dependency from classpath, and allows us to isolate this thing better, plus it is in my way right now as far as cleanups to core, due to some of the reflection it does (we can submit fixes, but still, lets isolate it).

@@ -51,3 +51,4 @@
- query:
{ "template": { "query": { "term": { "foo": { "value": "{{template}}" } } }, "params": { "template": "bar" } } }
- match: { responses.0.hits.total: 1 }

Copy link
Contributor

Choose a reason for hiding this comment

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

This particular test seems to be more testing msearch than mustache. I think it should stay in the main folder for rest tests minus the mustache part, and then we can have a dedicated mustache test for the msearch integration if we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, the test is messy, but so are many tests (see messy tests package).

If me (the guy simply refactoring stuff) has to fix all of these tests, i'm going to destabilize the build: guaranteed. Thats why I simply move stuff around: the onus cannot be on me.

Alternatively, I am happy to delete the test. But i guess I am saying, I do not think it should be fixed on this issue.

@jpountz
Copy link
Contributor

jpountz commented Dec 9, 2015

I left one comment about a rest test move but other than that it looks good to me

@rmuir
Copy link
Contributor Author

rmuir commented Dec 10, 2015

I renamed that rest test to 50_messy_test_msearch.yaml and added it to the messy tests list.

@jpountz
Copy link
Contributor

jpountz commented Dec 10, 2015

That works for me, thanks, it's more obvious the test is misplaced now. LGTM

rmuir added a commit that referenced this pull request Dec 10, 2015
Factor mustache -> modules/lang-mustache
@rmuir rmuir merged commit 1ef24d2 into elastic:master Dec 10, 2015
@rmuir rmuir removed the v2.2.0 label Dec 10, 2015
@clintongormley clintongormley added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Dec 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants