Improve documentation and add a test suite. #5
Conversation
The 'putMapping' action has a 'MappingName' argument for this purpose,
no need to also specify the name in the 'ToJSON' instance.
This bug broke upsert semantics -- inserting the same mapping twice
resulted in a "Root type mapping not empty after parsing!"
exception.
See
* https://www.elastic.co/guide/en/elasticsearch/reference/2.4/indices-put-mapping.html
* bitemyapp/bloodhound#143
According to
* https://www.elastic.co/guide/en/elasticsearch/reference/2.4/mapping-date-format.html
they are synonyms, but ES <2 doesn't support 'strict_date_time':
* https://www.elastic.co/guide/en/elasticsearch/reference/1.4/mapping-date-format.html
| "type" .= ("integer"::T.Text) | ||
| ] | ||
| , "insertion_time" .= object [ | ||
| "type" .= ("date"::T.Text) | ||
| , "format" .= ("strict_date_time"::T.Text) | ||
| , "format" .= ("date_time"::T.Text) |
There was a problem hiding this comment.
Is this for compatibility with ElasticSearch 1.7?
There was a problem hiding this comment.
Elasticsearch < 2 in general, which is the recommended version to use with Bloodhound.
There was a problem hiding this comment.
@23Skidoo another Bloodhound user is trying to make 2.x compatibility happen, but we're hesitant to leave the AWS users behind. Unfortunately, the hosted AWS Elasticsearch offering is 1.x only at the moment.
There was a problem hiding this comment.
@bitemyapp AWS supports both 1.5 and 2.3 now. Good to know that 2.x compatibility is being worked on.
There was a problem hiding this comment.
@23Skidoo not super actively. If it's something you want, you'll have to pitch in.
There was a problem hiding this comment.
@bitemyapp I'll keep it in mind, not sure right now how important 2.x support is for us.
There was a problem hiding this comment.
We could go with 2.x on AWS, since we haven't set anything up yet, and I have no idea how hard it is to migrate, but it sounds like something to avoid.
@arybczak reported no problems with 2.3.3 so maybe we should try that... especially if bloodhound support is in the pipeline.
| @@ -0,0 +1,83 @@ | |||
| # -*- restclient -*- | |||
There was a problem hiding this comment.
This file looks interesting, but I have no idea how it works!?
Could you enlighten the ignorant :)
There was a problem hiding this comment.
| @@ -1,2 +1,7 @@ | |||
| /dist/ | |||
| /dist-newstyle/ | |||
| /.cabal-sandbox/ | |||
| TAGS | |||
There was a problem hiding this comment.
Is this not better in your own global .gitignore? I don't know what the current "best practice" is... just a thought.
There was a problem hiding this comment.
With a global .gitignore you might end up ignoring more stuff than strictly needed as your global .gitignore becomes the union of .gitignores for all projects you're working on. Per-project ones also provide a better experience for new contributors.
|
Nice work adding some documentation to parameters, etc. Ups the quality and this is good. Will try this out in the evening, just minor comments from first glance for now. |
| @@ -22,15 +25,19 @@ before_cache: | |||
|
|
|||
| matrix: | |||
| include: | |||
| - env: CABALVER=head GHCVER=7.8.4 | |||
| - env: CABALVER=head GHCVER=7.8.4 ELASTICSEARCH=1.7 | |||
There was a problem hiding this comment.
Elasticsearch < 2 in general, which is the recommended version to use with Bloodhound.
In our own self interest should we test against Elasticsearch 1.5?
As this is what AWS supports, though Bloodhound already tests against 1.7 (and others too).
There was a problem hiding this comment.
FYI, I was working with 2.3.3 and it was just fine.
There was a problem hiding this comment.
@arybczak Yeah, it doesn't seem to be a problem even though it is not "officially" supported by Bloodhound.
|
Relevant upstream PR, just for reference: bitemyapp/bloodhound#143. |
Also fixes a couple of bugs uncovered by the test suite.